Skip to content

Conversation

jan-auer
Copy link
Contributor

@jan-auer jan-auer commented Oct 3, 2019

Taking a closer look at the test failures on beta it seems like repr(C) is broken now on Beta. The compiler introduces additional padding around u32 fields that are already aligned. There were a bunch of other warnings that are now fixed in this PR, but the core problem still exists.

@jan-auer
Copy link
Contributor Author

jan-auer commented Oct 3, 2019

Okay, still not quite sure what the correct fix for this is, but here's the reason for the regression on the example of the failing elf::dynamic test.

The problem is that Dyn somehow ends up in goblin::elf::dynamic, which gets publicly imported in dyn32 and shadows the 32-bit Dyn implementation.

@jan-auer
Copy link
Contributor Author

jan-auer commented Oct 3, 2019

I'm just not able to read code. Of course, Dyn was declared explicitly in goblin::elf::dynamic.

Due to some change in the beta compiler, the outer Dyn now shadows the inner one in dyn32. I still think this is to be considered a regression in the compiler, but independently, it's not clean do rely on this here. I updated the code to avoid shadowing now.

@jan-auer jan-auer changed the title Fix beta warnings and clippy lints Fix errors, warnings and lints on Beta Oct 3, 2019
@petrochenkov
Copy link

Less invasive workaround for the glob shadowing issue - rust-lang/rust#65090 (comment).

@m4b
Copy link
Owner

m4b commented Oct 5, 2019

Weird 😲 for either workaround id like a reference to the rustc issue so people aren’t confused and if it gets fixed we can go back to what it was ?

@petrochenkov
Copy link

petrochenkov commented Oct 5, 2019

id like a reference to the rustc issue

The link in the previous comment refers to that issue - rust-lang/rust#65090.

@m4b
Copy link
Owner

m4b commented Oct 5, 2019

This is a huge change with a huge diff can we just do petrochenkovs change ? I can push it

@m4b
Copy link
Owner

m4b commented Oct 5, 2019

I just meant a link in code for the fix :)

@jan-auer
Copy link
Contributor Author

jan-auer commented Oct 5, 2019

Uh yeah, sorry for the big diff. Contains lots of indentation. I agree that it’s a little large. If you prefer the other fix, I can change it.

On a personal note, I’m not a big fan of shadowing. It does not seem too clean and can lead to situations that are hard to debug (as seen in this instance).

@jan-auer
Copy link
Contributor Author

jan-auer commented Oct 5, 2019

Or feel free to change this PR :)

Please just retain the first commit so that we get the warnings fixed, too.

@m4b
Copy link
Owner

m4b commented Oct 7, 2019

@jan-auer I have applied @petrochenkov fix but I think you need to allow me to push onto your branch? Not sure; I tried various variations on:

git push jan-auer fix/beta

but my git skills probably need work. Do you know a quick command to push to your remote branch? Nothing seems to work for me:

(and jan-auer is git remote add jan-auer https://github.com/jan-auer/goblin)

@jan-auer
Copy link
Contributor Author

jan-auer commented Oct 7, 2019

@m4b not sure, sounds like this should have worked :)

Rebased the branch now.

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok wonderful, thanks so much!

@m4b m4b merged commit 6ced0de into m4b:master Oct 8, 2019
@jan-auer jan-auer deleted the fix/rust-beta branch May 6, 2020 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants