[llvm-commits] [PATCH] basic reading reloc visitor for x86_64 ELF

[Eli Bendersky]

>> >
>>
>> A slightly expanded version of this will make for a great comment in
>> the code :-) Seriously, please document the purpose of the new
>> relocMap data structure you're adding.
>>
>
> Uh hey, good point. Done :)

Great. At the moment I get a mental Parse Error when reading the
comment (especially the first sentence), though. I'll try to find time
to study the code better a bit later, maybe it will make the error go
away.

>> > Couple of areas that will need to be improved later:
>> >
>> > a) Relocations in more than a single section: the .debug_info section is
>> > the
>> > primary one I cared about first, however, we'll need either
>> >   1) A better mapping that contains section + address (since the debug
>> > sections are mapped at address 0 I can't just use total offset)
>> >   2) More mappings per section we're disassembling
>> >
>> > I'm likely to go with #2 rather than #1, but I'm open to any rationale
>> > either direction.
>> >
>>
>> I'm not sure I understood #2 - can you elaborate?
>
>
> Yeah, it'll look like this:
>   static DIContext *getDWARFContext(bool isLittleEndian,
>                                     StringRef infoSection,
>                                     StringRef abbrevSection,
>                                     StringRef aRangeSection = StringRef(),
>                                     StringRef lineSection = StringRef(),
>                                     StringRef stringSection = StringRef(),
>                                     StringRef rangeSection = StringRef(),
>                                     const RelocAddrMap &infoSectionMap =
> RelocAddrMap(),
>                                     const RelocAddrMap &aRangeSectionMap =
> RelocAddrMap(),
>                                     const RelocAddrMap &lineSectionMap =
> RelocAddrMap());
>
> instead of:
>
>   static DIContext *getDWARFContext(bool isLittleEndian,
>                                     StringRef infoSection,
>                                     StringRef abbrevSection,
>                                     StringRef aRangeSection = StringRef(),
>                                     StringRef lineSection = StringRef(),
>                                     StringRef stringSection = StringRef(),
>                                     StringRef rangeSection = StringRef(),
>                                     const RelocAddrMap &Map =
> RelocAddrMap());
>
> where I add a section specific map. It's more variables to a constructor and
> a bit more to look up, but more obvious what's going on. There's some
> complexity in deciding which is the proper map to grab at the right time,
> but that can be done as we go through the sections without needing to change
> the form parser.

Hmm, I don't know if it makes sense to flatten out all info about
every section like this. DWARF relocations can appear for a number of
sections and other things may be added in the future. However, it's
probably not worth over-thinking it at this point as it may be
refactored later if the need arises.


>> Do you have any tests for this?
>
>
> I mostly plan on using for writing tests for debug info, I can add a simple
> one that shows the proper strings and addresses output for a simple testcase
> though via the existing debug info tests that use dwarfdump and just make
> one use x86_64-elf. That about what you were thinking about?

Sounds good.

Eli