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

macro-issue1233.rs is broken on big-endian systems #1253

Closed
CohenArthur opened this issue May 17, 2022 · 2 comments
Closed

macro-issue1233.rs is broken on big-endian systems #1253

CohenArthur opened this issue May 17, 2022 · 2 comments

Comments

@CohenArthur
Copy link
Member

The issue probably comes to how we perform cfg expansion in blocks, regarding statements and final expressions.
Here is a test case failing as well on LE systems.

// { dg-additional-options "-w" }

macro_rules! impl_uint {
    ($($ty:ident = $lang:literal),*) => {
        $(
            impl $ty {
                pub fn to_le(self) -> Self {
                    #[cfg(not(target_endian = "big"))]
                    {
                        self
                    }
                    #[cfg(target_endian = "big")]
                    {
                        self
                    }
                }
            }
        )*
    }
}

impl_uint!(u8 = "u8", u16 = "u16", u32 = "u32");

The order of the "blocks + cfg" in the test case matter, which means that what is probably happening is we are parsing the first block + cfg as a statement. When the second block + cfg gets removed, due to the predicate not being met, we end up with a statement and no return expression. So the function gets a Self as a statement, ignores that, returns a (), and we get a type error.

And for an example that rustc can compile without complaining about redefining primitive items:

// { dg-additional-options "-w" }

macro_rules! impl_uint {
    ($($ty:ident = $lang:literal),*) => {
        $(
            struct $ty;

            impl $ty {
                pub fn to_le(self) -> Self {
                    #[cfg(not(target_endian = "big"))]
                    {
                        self
                    }
                    #[cfg(target_endian = "big")]
                    {
                        self
                    }
                }
            }
        )*
    }
}

impl_uint!(z1 = "z1", z16 = "z16", z12 = "z12");
@CohenArthur CohenArthur added this to the Imports and visibility milestone May 17, 2022
@CohenArthur CohenArthur added this to To do in Imports and Visbility via automation May 17, 2022
@CohenArthur
Copy link
Member Author

The corresponding struct in rustc simply contains one Vec<Stmt> and no optional tail expression: https://github.com/rust-lang/rust/blob/77972d2d0134fb597249b3b64dcf9510a790c34e/compiler/rustc_ast/src/ast.rs#L567-L583

CohenArthur added a commit to CohenArthur/gccrs that referenced this issue May 18, 2022
This testcase uncovered a very interesting bug requiring a refactor of
our `AST::Block` class (Rust-GCC#1253), but should still be fixed/adapted in the
meantime so that the BE builds on our buildbot do not fail.
bors bot added a commit that referenced this issue May 18, 2022
1252: privacy: Handle calls to functions defined in previous ancestors r=CohenArthur a=CohenArthur

Previously, we would only check if `current_module` was a direct
descendant of the item's module. However, we also need to visit each of this
item's module's children recursively.


1254: issue #1233: Do not rely on the endianness for testing r=CohenArthur a=CohenArthur

This testcase uncovered a very interesting bug requiring a refactor of
our `AST::Block` class (#1253), but should still be fixed/adapted in the
meantime so that the BE builds on our buildbot do not fail.

I've tested this newtestcase with a compiler from 74e8365, which was the commit pointed out in #1233. The same ICE would still trigger, so I can safely say that this is a different exemple showing the same underlying issue. I'll work on fixing #1253 but it is a refactor we need to think about a little.

This should make all the architectures on buildbot happy again!

Co-authored-by: Arthur Cohen <arthur.cohen@embecosm.com>
CohenArthur added a commit to CohenArthur/gccrs that referenced this issue May 20, 2022
This testcase uncovered a very interesting bug requiring a refactor of
our `AST::Block` class (Rust-GCC#1253), but should still be fixed/adapted in the
meantime so that the BE builds on our buildbot do not fail.
@philberty philberty removed this from To do in Imports and Visbility May 25, 2022
@philberty philberty removed this from the Imports and visibility milestone May 25, 2022
@CohenArthur
Copy link
Member Author

This should be fixed by #2156 as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

2 participants