EhHdrTable::lookup requires a base address to .eh_frame_hdr in BaseAdresses::data instead of a base address to .data as the docs suggest

[koute]

I've been adding support for .eh_frame_hdr to our profiler; after a lot of debugging and tweaking I've managed to get it to work, but it was very confusing.

First and foremost, currently EhHdrTable::lookup's prototype looks like this:

pub fn parse(&self, bases: &BaseAddresses, addr_size: u8) -> Result<ParsedEhFrameHdr<R>>

I see this and think - great! Let's initialize the bases like I usually do - set_text with the base address to .text, set_data with the base address to .data and set_cfi with the base address to .eh_frame. However, this didn't work - EhHdrTable::lookup (and consequently also EhHdrTable::lookup_and_parse since it uses lookup) returned incorrect results! So after a lot of trial-and-error I got it to actually work correctly with BaseAddresses initialized like this:

let mut bases = BaseAddresses::default();
bases = bases.set_cfi( eh_frame_base );
bases = bases.set_data( eh_frame_hdr_base );

The docs for gimli::BaseAddresses::data say that:

The address of the .data section in memory.

...and yet here I am setting it to the base address of the .eh_frame_hdr instead of .data. Is this correct? Or is this a bug in gimli?

Another minor issue is that the docs for gimli::BaseAddresses::cfi say that:

The address of the current CFI unwind section (.eh_frame or .debug_frame) in memory.

However EhFrameHdr::parse requires the cfi to be the address to .eh_frame_hdr, which is not obvious from the docs.

[philipc]

#316 (comment) is related I think, and I should have opened a bug for it at the time. So yeah, this is too easy to get wrong and we should fix it somehow.

[koute]

Perhaps we could have (at least in case of e.g. EhHdrTable::lookup) replace the generic BaseAddresses with a non-generic struct for which it would be obvious what is what? (Have set_eh_frame and set_eh_frame_hdr instead of set_cfi and set_data.)

[philipc]

I think we only need to add set_eh_frame_hdr. Keep set_text, set_data, and set_cfi (don't call it set_eh_frame because it needs to allow for .debug_frame too). Internally, parse_encoded_pointer will need something to indicate which base to use.

The fact that you had to call bases.set_data( eh_frame_hdr_base ) is really weird and needs looking into further... I was only expecting there to be confusion between the .eh_frame and .eh_frame_hdr base addresses, not the .data base address as well.

[philipc]

Ok so ignore my previous comment, I hadn't looked into how this worked enough. From what I can tell, these pointer encodings are only used with .eh_frame style augmentations. And the documentation for gimli::BaseAddresses::data is clearly wrong because the LSB says that DW_EH_PE_datarel is relative to the beginning of the .got or .eh_frame_hdr section.

Have set_eh_frame and set_eh_frame_hdr instead of set_cfi and set_data.

So based on my understanding now, this sounds to me like the right thing to do in order to handle DW_EH_PE_datarel correctly.

I think this is a separate issue from what I was talking about in #316 (comment) though, and the above isn't enough to fix DW_EH_PE_pcrel too. For that parse_encoded_pointer will still need something to indicate which base to use for DW_EH_PE_pcrel. The issue here is that gimli's Reader doesn't support seeking backwards, so we use a hack to do the PC relative seek. Not sure if these ever occur in .eh_frame_hdr though.

[philipc]

Not sure if these ever occur in .eh_frame_hdr though.

And they do for the eh_frame_ptr field in the .eh_frame_hdr (which is the other issue you listed).