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

Adjust default DW_EH_PE_pcrel calculation #576

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

alexcrichton
Copy link
Contributor

Today I was fiddling around in Wasmtime with how we structure and emit
our .eh_frame section for JIT code, and my goal was to avoid the need
to generate .eh_frame every time a module is loaded as it is today.
Because the FDEs have absolute addresses in them right now though I
needed to figure out some sort of relative solution so I could just make
sure that the .eh_frame section and the JIT code memory were
relatively in the same place.

This exploration led me to stumble upon DW_EH_PE_pcrel. That seemed to
be roughly what I wanted where the description of what an FDE was
modifying was relative to the address of the FDE itself. Upon digging
into gimli's implementation of DW_EH_PE_pcrel handling it currently
factors in the offset of where the pointer is encoded itself. I assume
that this is because applications generally don't know where the FDE
address is encoded but they do know, for example, where the start of the
.eh_frame will be encoded.

I forged ahead with trying to use this but was then a bit confused when
unwinding didn't actually work out. Upon actually trying to do the math,
it appears that what gimli does today is:

actual_encoded_value = offset_of_fde - provided_value

For my use case I knew that the distance between the start of the text
section and the .eh_frame is a constant N, so what I was shooting
for was:

actual_encoded_value = -(N - offset_in_text) - offset_of_fde

where for me provided_value = -(N - offset_in_text). I noticed that
the subtraction here is backwards, and I am submitting this PR because I
think that may be a mistake but I could also be mistaken! I'm well known
to get various minus signs here mixed up and such...

Today I was fiddling around in Wasmtime with how we structure and emit
our `.eh_frame` section for JIT code, and my goal was to avoid the need
to generate `.eh_frame` every time a module is loaded as it is today.
Because the FDEs have absolute addresses in them right now though I
needed to figure out some sort of relative solution so I could just make
sure that the `.eh_frame` section and the JIT code memory were
relatively in the same place.

This exploration led me to stumble upon `DW_EH_PE_pcrel`. That seemed to
be roughly what I wanted where the description of what an FDE was
modifying was relative to the address of the FDE itself. Upon digging
into `gimli`'s implementation of `DW_EH_PE_pcrel` handling it currently
factors in the offset of where the pointer is encoded itself. I assume
that this is because applications generally don't know where the FDE
address is encoded but they do know, for example, where the start of the
`.eh_frame` will be encoded.

I forged ahead with trying to use this but was then a bit confused when
unwinding didn't actually work out. Upon actually trying to do the math,
it appears that what gimli does today is:

    actual_encoded_value = offset_of_fde - provided_value

For my use case I knew that the distance between the start of the text
section and the `.eh_frame` is a constant `N`, so what I was shooting
for was:

    actual_encoded_value = -(N - offset_in_text) - offset_of_fde

where for me `provided_value = -(N - offset_in_text)`. I noticed that
the subtraction here is backwards, and I am submitting this PR because I
think that may be a mistake but I could also be mistaken! I'm well known
to get various minus signs here mixed up and such...
@alexcrichton
Copy link
Contributor Author

To clarify on this, I find this stuff notoriously hard to keep straight in my head so this is a bit of a shot in the dark assuming that the swapped order here was intended. At least for me with the swap my tests are all passing as expected, but I could also very well be missing the intended purpose of this calculation.

Copy link
Collaborator

@philipc philipc left a comment

Choose a reason for hiding this comment

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

Oops, this is a bug. Looks like you're the first person to use this code (I've always overridden this for relocations).

Can you also tell me if you think that TODO: better handling of sign needs anything done for it? I can't remember why I added that. Maybe it's because these are actually i32 or i64 values, but I think it should still just work as is.

@philipc philipc merged commit ea995a2 into gimli-rs:master Aug 12, 2021
@philipc
Copy link
Collaborator

philipc commented Aug 12, 2021

Also, write_eh_pointer possibly should also be factoring in the address of .eh_frame, but we don't have that information anywhere currently, so as long as you're consistently giving all Address values relative to the address of .eh_frame, it should be okay. Another option would be for you to override write_eh_pointer to include that calculation yourself.

@alexcrichton alexcrichton deleted the switch-pcrel branch August 12, 2021 14:41
@alexcrichton
Copy link
Contributor Author

For the sign handling I think it's fine as-is, you need to specify either i64 or u64 as the type and u64 seems like a reasonable choice, but with the wrapping part of the subtraction I don't think it matters whether it's signed or unsigned. I haven't thought too deeploy about the 32-bit implications though...

I ended up using an overwritten write_eh_pointer for this, yeah, but in the long run I was looking at the source of libunwind for runtime-registration of unwinding tables and I think that this is the only relative mode that's supported, so at least for my use case I just need to make sure that the address of the start of .eh_frame is well known relative to the start of the code that actually might unwind.

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.

None yet

2 participants