[PATCH] D137882: [DWARFLibrary] Add support to re-construct cu-index

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 2 16:31:12 PST 2022


dblaikie added a comment.

In D137882#3967905 <https://reviews.llvm.org/D137882#3967905>, @ayermolo wrote:

> In D137882#3967890 <https://reviews.llvm.org/D137882#3967890>, @dblaikie wrote:
>
>> In D137882#3967704 <https://reviews.llvm.org/D137882#3967704>, @ayermolo wrote:
>>
>>> bin/llvm-lit -a /data/users/ayermolo/server-llvm/llvm-project/llvm/test/tools/llvm-dwp/X86/invalid-cu-index.s
>>>
>>> F25484289: image.png <https://reviews.llvm.org/F25484289>
>>>
>>> bin/llvm-dwarfdump --manaully-generate-unit-index --debug-cu-index /data/users/ayermolo/llvm-build-release/test/tools/llvm-dwp/X86/Output/invalid-cu-index.s.tmp.dwp
>>> F25484297: image.png <https://reviews.llvm.org/F25484297>
>>> bin/llvm-dwarfdump --debug-cu-index /data/users/ayermolo/llvm-build-release/test/tools/llvm-dwp/X86/Output/invalid-cu-index.s.tmp.dwp
>>> F25484301: image.png <https://reviews.llvm.org/F25484301>
>>
>> These don't quite look like how I'd expect. I'd have thought the manually-generate-unit-index would be different/correct, showing the value in the manual generation over the pre-built but buggy/overflowed index?
>>
>>>   # This test checks that with invalid offset in the cu index
>>>   # we can reconstruct it manually.
>>>   
>>>   # RUN: llvm-mc --filetype=obj --triple x86_64 %s -o %t.dwp
>>>   # RUN: llvm-dwarfdump --manaully-generate-unit-index --debug-cu-index %t.dwp | FileCheck %s
>>>   
>>>   # This test checks that we parse correctly cu-index that has entries over 4GB.
>>>   # It is setup to work with current llvm implementation where cu-index is 32bit.
>>>   # Once we move to 64bit internal representation, it will need to be modified.
>>
>> Sorry, I'm not following here ^ could you explain in more detail/rephrase?
>
> Well internal data structure for SectionContribution is still 32bit. So right now even with parsing we are still restricted by that. So output won't change.
>
>   struct SectionContribution {
>         uint32_t Offset;
>         uint32_t Length;
>       };
>
> My next patch will be changing it to 64bit and add accessors, but I am waiting for https://reviews.llvm.org/D138618 to go through review and land before posting it. 
> Maybe it's wrong order? Put up second patch changing data structure to 64bit, and in lldb code return 32 bit until that review goes through?
>
> Does this clarify it for your second question, or should I try to rephrase?

Ah, that makes this patch a bit untestable, though - perhaps this patch should wait for the 64bit underlying change, then this patch will have a real purpose/make a difference, where today it doesn't change anything. (committing things that don't change anything, then committing some otherwise-cleanup-ish refactoring that enables the previous change is tricky, because then it's hard to keep track of things being tested, since they couldn't be tested when they were committed owing to the missing underlying refactoring/changes - though it can be a chicke-and-egg situation, in this case I think it's probably clear enough that changing 32 bit offsets to 64 bit ones is pretty benign/mechanical & could be done/reviewed relatively easily (relatively, still lots of changes) & then this change would go on top making the behavior change/tested (albeit manually), etc)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137882/new/

https://reviews.llvm.org/D137882



More information about the llvm-commits mailing list