Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Star Import Precedence Regression in Beta #65090

Closed
mitsuhiko opened this issue Oct 4, 2019 · 13 comments
Closed

Star Import Precedence Regression in Beta #65090

mitsuhiko opened this issue Oct 4, 2019 · 13 comments

Comments

@mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented Oct 4, 2019

The goblin tests started failing in beta because there seem to be a change in order for how star imports are applied. This PR changes the imports so nothing shadows any more: m4b/goblin#184

Annoyingly I cannot produce an isolated test case that shows this behavior.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

@petrochenkov petrochenkov commented Oct 4, 2019

Doing a bisection here would be very helpful.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

@petrochenkov petrochenkov commented Oct 4, 2019

I don't recall any changes to globs specifically, so perhaps this is an accidental effect from some other change.

@mitsuhiko

This comment has been minimized.

Copy link
Contributor Author

@mitsuhiko mitsuhiko commented Oct 4, 2019

I'm completely at a loss here. I thought I could reproduce it with this, but I cannot. The issue comes the moment the dynamic::Dyn gets imported into the inner modules that are created by the macro expansion but an isolated test does not show it.

I'm also not sure how this can be bisected.

@mitsuhiko

This comment has been minimized.

Copy link
Contributor Author

@mitsuhiko mitsuhiko commented Oct 4, 2019

This is a minimal repo. It requires the SizeWith macro from the scroll crate:

macro_rules! def_struct {
    () => {
        // The import here is important cannot inline scroll::SizeWith
        use scroll::SizeWith;
        #[derive(Copy, Clone, SizeWith)]
        pub struct Test {
            pub a: u32,
            pub b: u32,
        }
    };
}

pub struct Test {
    pub a: u64,
    pub b: u64,
}

macro_rules! def_test {
    () => {
        #[cfg(test)]
        mod tests {
            use super::*;
            #[test]
            fn size_of() {
                assert_eq!(::std::mem::size_of::<Test>(), 8);
            }
        }
    };
}

pub mod dyn32 {
    pub use super::*;

    def_struct!();
    def_test!();
}

cargo-expand claims this expands to the following

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std;
pub struct Test {
    pub a: u64,
    pub b: u64,
}
pub mod dyn32 {
    pub use super::*;
    use scroll::SizeWith;
    pub struct Test {
        pub a: u32,
        pub b: u32,
    }
    #[automatically_derived]
    #[allow(unused_qualifications)]
    impl ::core::marker::Copy for Test {}
    #[automatically_derived]
    #[allow(unused_qualifications)]
    impl ::core::clone::Clone for Test {
        #[inline]
        fn clone(&self) -> Test {
            {
                let _: ::core::clone::AssertParamIsClone<u32>;
                let _: ::core::clone::AssertParamIsClone<u32>;
                *self
            }
        }
    }
    impl ::scroll::ctx::SizeWith<::scroll::Endian> for Test {
        type Units = usize;
        #[inline]
        fn size_with(ctx: &::scroll::Endian) -> Self::Units {
            0 + <u32>::size_with(ctx) + <u32>::size_with(ctx)
        }
    }
}
@mitsuhiko

This comment has been minimized.

Copy link
Contributor Author

@mitsuhiko mitsuhiko commented Oct 4, 2019

This to me looks like a bug that was fixed actually. Because removing the def_test! macro and inlining what it expands to makes this code also fail on stable.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

@petrochenkov petrochenkov commented Oct 4, 2019

Thanks for minimizing, I'll try to look at this during the weekend.

FWIW, we have existing bugs (not regressions) with globs that try to import something macro-expanded (#56593), perhaps some change made that bug to appear (or disappear) for this code.

@petrochenkov petrochenkov self-assigned this Oct 4, 2019
@petrochenkov

This comment has been minimized.

Copy link
Contributor

@petrochenkov petrochenkov commented Oct 5, 2019

Further minimized:

struct S { // Outer
    field: u8,
}

macro_rules! def_inner_s {
    () => {
        use std::clone::Clone;
        #[derive(Clone)]
        struct S {} // Inner
    };
}

macro_rules! def_test {
    () => {
        use super::*;
        fn test() {
            S {}; // works on stable, reports a missing field on beta
        }
    };
}

mod outer {
    use super::*;

    def_inner_s!();

    mod inner {
        def_test!();
    }
}

fn main() {}
@petrochenkov

This comment has been minimized.

Copy link
Contributor

@petrochenkov petrochenkov commented Oct 5, 2019

The root cause of the issue is that glob imports don't update their lists of imported names properly. Import use m::* may snapshot the content of m at one moment and may ignore later updates to m caused e.g. by expanded macros.
This is an old issue and not a regression, #56593 should be the primary issue for it right now.

The regression here is that some expansion order changed, which should be harmless by itself, but the new expansion order triggers the root issue with glob import.
With the new order the inner S materializes a bit later and doesn't get into the glob's "snapshot".

The PR changing the expansion order was either #63248 or #63867 with high probability. (Both PRs are correct in isolation, if we didn't have the glob bug.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

@petrochenkov petrochenkov commented Oct 5, 2019

We cannot revert #63248 or #63867, so the fix amounts to implementing globs properly.
I always assumed that to be a big task requiring investigation and possibly a major rewrite.
I could be wrong though, I'll try to look at the code and make some estimation.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

@petrochenkov petrochenkov commented Oct 5, 2019

The workaround for goblin is to make struct Dyn and struct SectionHeader to expand slightly earlier, e.g. like this:

diff --git a/src/elf/dynamic.rs b/src/elf/dynamic.rs
index 52ca0bd..30540a5 100644
--- a/src/elf/dynamic.rs
+++ b/src/elf/dynamic.rs
@@ -1,11 +1,9 @@

 macro_rules! elf_dyn {
     ($size:ty) => {
-        #[cfg(feature = "alloc")]
-        use scroll::{Pread, Pwrite, SizeWith};
         #[repr(C)]
         #[derive(Copy, Clone, PartialEq, Default)]
-        #[cfg_attr(feature = "alloc", derive(Pread, Pwrite, SizeWith))]
+        #[cfg_attr(feature = "alloc", derive(scroll::Pread, scroll::Pwrite, scroll::SizeWith))]
         /// An entry in the dynamic array
         pub struct Dyn {
             /// Dynamic entry type
diff --git a/src/elf/section_header.rs b/src/elf/section_header.rs
index 2eb9143..bbbe88d 100644
--- a/src/elf/section_header.rs
+++ b/src/elf/section_header.rs
@@ -1,10 +1,8 @@
 macro_rules! elf_section_header {
     ($size:ident) => {
-        #[cfg(feature = "alloc")]
-        use scroll::{Pread, Pwrite, SizeWith};
         #[repr(C)]
         #[derive(Copy, Clone, Eq, PartialEq, Default)]
-        #[cfg_attr(feature = "alloc", derive(Pread, Pwrite, SizeWith))]
+        #[cfg_attr(feature = "alloc", derive(scroll::Pread, scroll::Pwrite, scroll::SizeWith))]
         /// Section Headers are typically used by humans and static linkers for additional information or how to relocate the object
         ///
         /// **NOTE** section headers are strippable from a binary without any loss of portability/executability; _do not_ rely on them being there!
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 10, 2019

visit for triage: Marking P-medium because this does not sound like a "true regression" or at least not an urgent one. Leaving nomination label on so that T-compiler can discuss at triage meeting to confirm that priority assignment.

@pnkfelix pnkfelix added the P-medium label Oct 10, 2019
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 17, 2019

I raised the issue at last week's meeting. No one objected to the P-medium prioritization.

removing nomination label.

Also, we may want to simply close this and let #56593 act as the representative issue for "globs gots bugs"

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 31, 2019

closing; letting #56593 serve as representative for this bug, and we are going to let this regression slide through to stable.

@pnkfelix pnkfelix closed this Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.