Logging unsafe removal #9593

fhahn · 2013-09-28T18:14:27Z

This pull request changes to memory layout of the CrateMap struct to use static slices instead of raw pointers. Most of the discussion took place here .

The memory layout of CrateMap changed, without bumping the version number in the struct. Another, more backward compatible, solution would be to keep the old code and increase the version number in the new struct. On the other hand, the annihilate_fn pointer was removed without bumping the version number recently.

At the moment, the stage0 compiler does not use the new memory layout, which would lead the segfaults during stage0 compilation, so I've added a dummy iter_crate_map function for stage0, which does nothing. Again, this could be avoided if we'd bump the version number in the struct and keep the old code.

I'd like to use a normal for loop here,

    for child in children.iter() {
        do_iter_crate_map(child, |x| f(x), visited);
    }

but for some reason this only yields error: unresolved enum variant, struct or const 'Some' and I have no idea why.

sfackler · 2013-09-28T19:17:58Z

for val in thing {...} loops expand to something like

loop {
    match thing.next() {
        Some(val) => {...}
        None => break
    }
}

and the prelude isn't imported inside of libstd. If you manually use option::{Some, None}, it should work.

fhahn · 2013-09-28T21:45:24Z

Thanks, should be updated now!

huonw · 2013-09-29T00:08:35Z

src/libstd/rt/crate_map.rs


-    struct CrateMapT3 {
+    struct CrateMap<'self> {


We could use this definition above, rather than duplicating them. (I think moving this above would only require changing the static declaration in the extern block, and possibly the signature of the windows function for retrieving the CrateMap.)

huonw · 2013-09-29T00:18:18Z

Very cool! Much much less unsafe now! 😄

luqmana · 2013-09-29T01:42:53Z

src/libstd/rt/crate_map.rs

 }

 #[cfg(windows)]
 #[fixed_stack_segment]
 #[inline(never)]
-pub fn get_crate_map() -> *CrateMap {
+pub fn get_crate_map() -> &'static CrateMap {


I think this should give back Option<&'static CrateMap> and rt::logging::init should basically ignore the None case.

That would make things like this not segfault https://gist.github.com/luqmana/6748487

I'm not sure, in which case get_crate_map should return None?

The way get_crate_map works right now is that it relies on a _rust_crate_map_toplevel symbol existing in the final executable that libstd gets linked against. This symbol is emitted by rustc when you're compiling an executable crate. But there are valid uses cases in which the symbol won't exist like in the gist I linked where you have a rust library that exports an extern fn that starts up the runtime by itself which is called by some C program. In that case, the final executable isn't a rust executable and hence the cratemap symbol doesn't exist and thus would be NULL causing a segfault right now since the logging setup currently assumes it is never NULL.

fhahn · 2013-09-29T13:29:24Z

Thanks for the feedback.
I've pushed two commits with the changes suggested by @huonw.

However, it seems like get_crate_map returns a reference to a wrong root crate map. The following tests src/test/run-pass/conditional-debug-macro-on.rs fails for me with the current master. Compiling it with RUST_LOG=::help returns a lot of crates with a rustc prefix, but no conditional-debug-macro crate. Could it be that get_crate_map returns a reference to the crate map of rustc?
I'll want to have a closer look at get_crate_map before I'll add @luqmana changes.

huonw · 2013-09-29T13:37:14Z

If it's failing with the current master (i.e. even without your changes), then it might be a regression of #7898 (or something like that), possibly due to #9278.

(I'd guess this is almost certainly the case if that's the only test that is failing.)

huonw · 2013-09-29T13:38:07Z

What's your ./configure invocation?

(Also, the most recent changes look good.)

fhahn · 2013-09-29T22:43:06Z

After rebasing to the current master, all tests pass.

Another point I'm looking into is converting name in ModEntry to something like `&' self str' . Do static strings have the same memory layout as a slice?

huonw · 2013-09-29T22:55:35Z

Yes, &str is a non-null-terminated UTF-8-encoded &[u8].

alexcrichton · 2013-09-29T22:56:39Z

src/libstd/rt/crate_map.rs

-    }
+#[cfg(stage0)]
+/// Iterates recursively over `crate_map` and all child crate maps
+pub fn iter_crate_map<'a>(crate_map: &'a CrateMap<'a>, f: &fn(&ModEntry)) {


This seems like a bad idea to turn off all stage1 logging. It doesn't seem like you need the stage0 guard here?

After changing the memory layout of CrateMap the new code will segfault when compiled with the older stage0 compiler. We could bump the version number in the structure and keep the old code?

Oh I see, it still seems a shame to lose all stage1 logging because I imagine it's used quite frequently.

Take a snap soon after this lands?

This or bumping the version number in the structure (I guess it was intended for cases like this)

Let's bump the version number, I'll try to make a snap after this lands, but no guarantees.

... with a goal to remove the old code after the next snapshot. (Since we're still not backward-compatible, I think we should take the opportunity to make the code as nice as possible now, while we still don't have handle logging from legacy libs.)

I'll look into adding the old code again and add a note to delete it after the next snapshot. Btw, then we pobably could remove the CrateMapV0 struct as well?

I don't see why not, I'd want to make sure with others, though, first.

fhahn · 2013-09-30T21:35:21Z

I've pushed another commit, replacing name *c_char with &'self str.

I hope I'll be able to add the backward compatible code for the previous versions of CrateMap and the change suggested by @luqmana tomorrow.

fhahn · 2013-10-03T08:20:24Z

I've added the code for older versions of the CrateMap struct and the old test cases.

get_crate_map now returns Option<&'static CrateMap> as suggested. I tested it with the example provided earlier, https://gist.github.com/luqmana/6748487 and it woks with Linux, but I couldn't test it on Windows or any other platform.
I think it would make sense to add the test case above to the unit tests, but I'm not sure how to use rt::init() from another Rust file (for rewriting foo.c in Rust)

huonw · 2013-10-03T13:24:46Z

src/libstd/rt/crate_map.rs

+    version: i32,
+    entries: &'self [ModEntry<'self>],
+    /// a dynamically sized struct, where all pointers to children are listed adjacent
+    /// to the struct, terminated with NULL


This comment needs updating.

I've removed it, the code should be expressive enough now I think.

alexcrichton · 2013-10-03T16:32:10Z

This is pretty awesome, nice work! I'd be OK with some rebasings as well to clean up the commit history, but I don't really consider that a blocker either.

Once we warn on a non-empty RUST_LOG with a None crate map, I think this is good to go.

fhahn · 2013-10-04T16:22:10Z

I've added the warning.

Should I rebase everything in a big commit? I tried to address a part of the rewrite with every commit, but some could be squashed.

alexcrichton · 2013-10-04T18:05:14Z

src/libstd/rt/logging.rs

+            }
+        },
+        _ => { 
+            dumb_println("warning: RUST_LOG set, but no crate map found."); 


This isn't necessarily true I think that you still need to test that os::getenv returns Some

Of course, I think I did push it too soon, sorry for that!

alexcrichton · 2013-10-04T18:05:45Z

I don't think that this necessarily needs to become one large commit, but 11 seems a little excessive. Perhaps squashing a few of the intermediate commits?

Relaxe lifetime of CrateMap as well.

fhahn · 2013-10-06T08:51:03Z

I think fixed the failure on windows (this cast is required, because sym is a *c_void pointer).

But I have no idea why the other builds got interrupted during compilation (eg, http://buildbot.rust-lang.org/builders/auto-linux-64-nopt-t/builds/968/steps/compile/logs/stdio )

huonw · 2013-10-06T08:55:21Z

If a build fails the other builds get interrupted, so that we're not wasting the CPU time.

fhahn · 2013-10-06T09:43:23Z

A thanks. It seems like there was something wrong with the #cfg flags for windows, should be fixed now.

This pull request changes to memory layout of the `CrateMap` struct to use static slices instead of raw pointers. Most of the discussion took place [here](fhahn@63b5975#L1R92) . The memory layout of CrateMap changed, without bumping the version number in the struct. Another, more backward compatible, solution would be to keep the old code and increase the version number in the new struct. On the other hand, the `annihilate_fn` pointer was removed without bumping the version number recently. At the moment, the stage0 compiler does not use the new memory layout, which would lead the segfaults during stage0 compilation, so I've added a dummy `iter_crate_map` function for stage0, which does nothing. Again, this could be avoided if we'd bump the version number in the struct and keep the old code. I'd like to use a normal `for` loop [here](https://github.com/fhahn/rust/compare/logging-unsafe-removal?expand=1#L1R109), for child in children.iter() { do_iter_crate_map(child, |x| f(x), visited); } but for some reason this only yields `error: unresolved enum variant, struct or const 'Some'` and I have no idea why.

fhahn · 2013-10-06T16:04:21Z

Finally, thanks for all the feedback and patience.

luqmana · 2013-10-06T19:06:44Z

@fhahn Thank you! 😀

…hton This patch removes the code responsible for handling older CrateMap versions (as discussed during #9593). Only the new (safer) layout is supported now.

lint::unsafe_removed_from_name: fix false positive result when allowed changelog: [`unsafe_removed_from_name`] Fix allowing on imports produces a false positive on `useless_attribute`. Fixes: rust-lang#9197 Signed-off-by: Andy-Python-Programmer <andypythonappdeveloper@gmail.com>

huonw reviewed Sep 29, 2013
View reviewed changes

luqmana reviewed Sep 29, 2013
View reviewed changes

alexcrichton reviewed Sep 29, 2013
View reviewed changes

huonw mentioned this pull request Sep 30, 2013

Enable tests on Android bot #9120

Closed

huonw reviewed Oct 3, 2013
View reviewed changes

alexcrichton mentioned this pull request Oct 3, 2013

Android segfaults for API level <18 #9700

Closed

alexcrichton reviewed Oct 4, 2013
View reviewed changes

fhahn added 4 commits October 5, 2013 12:09

Use slice representation for child crate maps

787f20a

Use slice representation for module entries in CrateMap

9ef4463

Relaxe lifetime of CrateMap as well.

Use &'self str instead of raw char pointer in ModEntry

5dd1145

Add code for older crate map versions, bumped crate map version number

b7b4f7a

get_crate_map returns an Option

23176fc

bors closed this Oct 6, 2013

bors merged commit 23176fc into rust-lang:master Oct 6, 2013

fhahn deleted the logging-unsafe-removal branch October 6, 2013 16:03

fhahn mentioned this pull request Oct 11, 2013

Remove support for older CrateMap versions #9809

Merged

Logging unsafe removal #9593

Logging unsafe removal #9593

Conversation

fhahn commented Sep 28, 2013

sfackler commented Sep 28, 2013

fhahn commented Sep 28, 2013

Choose a reason for hiding this comment

huonw commented Sep 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn commented Sep 29, 2013

huonw commented Sep 29, 2013

huonw commented Sep 29, 2013

fhahn commented Sep 29, 2013

huonw commented Sep 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn commented Sep 30, 2013

fhahn commented Oct 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Oct 3, 2013

fhahn commented Oct 4, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Oct 4, 2013

fhahn commented Oct 6, 2013

huonw commented Oct 6, 2013

fhahn commented Oct 6, 2013

fhahn commented Oct 6, 2013

luqmana commented Oct 6, 2013