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

use NativeEndian in symbolize::gimli::Context #373

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Oct 8, 2020

Object uses NativeEndian, so the Context should too.

Cc: rust-lang/rust#77410

`Object` uses `NativeEndian`, so the `Context` should too.

Cc: rust-lang/rust#77410
@@ -5,7 +5,7 @@
//! intended to wholesale replace the `libbacktrace.rs` implementation.
Copy link
Member

Choose a reason for hiding this comment

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

Should this comment be updated? Isn't gimli the default now? (Not necessary to do here, just wondering.)

@cuviper
Copy link
Member Author

cuviper commented Oct 8, 2020

I do see big-endian powerpc64 in CI, so I'm not sure how this has been passing -- does it really run the tests?

I used native s390x to confirm the problem and this fix.

@cuviper
Copy link
Member Author

cuviper commented Oct 8, 2020

Oh:

# TODO: should actually run these tests
#CARGO_TARGET_POWERPC64_UNKNOWN_LINUX_GNU_RUNNER="qemu-ppc64 -L /usr/powerpc64-linux-gnu" \
CARGO_TARGET_POWERPC64_UNKNOWN_LINUX_GNU_RUNNER=echo \

@alexcrichton
Copy link
Member

Thanks! You've confirmed locally this fixes the issue too?

I actually thought that ELF was always little-endian for common platforms, regardless of the endianness of the platform itself, but this would make sense why things are broken on big-endian!

@philipc
Copy link
Contributor

philipc commented Oct 8, 2020

I actually thought that ELF was always little-endian for common platforms, regardless of the endianness of the platform itself

I think that's true for PE but not for ELF.

@cuviper
Copy link
Member Author

cuviper commented Oct 8, 2020

Thanks! You've confirmed locally this fixes the issue too?

On s390x, yes.

I actually thought that ELF was always little-endian for common platforms, regardless of the endianness of the platform itself, but this would make sense why things are broken on big-endian!

AFAIK it's always native in ELF, and from a quick skim of glibc it looks like the loader asserts that they match:
https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/dl-load.c;h=0c8fa72c4d1d2396f956ce3eb64c02b425b6fd22;hb=HEAD#l1713

Any recollection why that powerpc64 CI has the qemu-ppc64 line commented out?

@cuviper
Copy link
Member Author

cuviper commented Oct 8, 2020

I think that's true for PE but not for ELF.

Hmm, then should we make this import part of the platform-specific cfg_if?

@philipc
Copy link
Contributor

philipc commented Oct 8, 2020

Hmm, then should we make this import part of the platform-specific cfg_if?

This import seems to only be used for the DWARF, and I don't know what endianness DWARF in PE uses. My intuition is that NativeEndian would be correct for it too.

@alexcrichton
Copy link
Member

Ok sounds good! I'm gonna go ahead and merge this since it fixes an issue for rust-lang/rust, we can sort out CI afterwards if necessary. PE I think only affects Windows (right?) and while Gimli is used for MinGW targets I don't think that there's any big-endian Windows targets right now.

Any recollection why that powerpc64 CI has the qemu-ppc64 line commented out?

Ah I'm sure at some point in time in the past it was broken, but it was most likely a bug in qemu. If it works now seems good to uncomment!

@alexcrichton alexcrichton merged commit 616407e into rust-lang:master Oct 8, 2020
@mati865
Copy link

mati865 commented Oct 8, 2020

I think that's true for PE but not for ELF.

Correct, docs: https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#endianness

There are no official Rust MinGW ARM targets yet.
There are C/C++ MinGW ARM/Aarch64 targets though and some folks would like to see official Rust target already.

@alexcrichton
Copy link
Member

I'm pretty sure arm/aarch64 are little endian...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2020
…richton

Update the backtrace crate to fix big-endian ELF

Pulls in rust-lang/backtrace-rs#373.
Fixes rust-lang#77410.

r? `@alexcrichton`
@nagisa
Copy link
Member

nagisa commented Oct 10, 2020

I'm pretty sure arm/aarch64 are little endian...

ARM-the-architecture can go both ways, though its not awfully useful as the ecosystem around it pretty much universally uses LE. Context.

@camelid camelid mentioned this pull request Oct 29, 2020
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.

6 participants