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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> In D137882#3965250 <https://reviews.llvm.org/D137882#3965250>, @dblaikie wrote:
>
>> In D137882#3961341 <https://reviews.llvm.org/D137882#3961341>, @ayermolo wrote:
>>
>>> In D137882#3957811 <https://reviews.llvm.org/D137882#3957811>, @dblaikie wrote:
>>>
>>>> In D137882#3949594 <https://reviews.llvm.org/D137882#3949594>, @ayermolo wrote:
>>>>
>>>>> In D137882#3948380 <https://reviews.llvm.org/D137882#3948380>, @dblaikie wrote:
>>>>>
>>>>>> I think test coverage might be more suitable if it were a single dedicated test that has a corrupted index, to demonstrate that rebuilding the index comes up with a different (& correct) answer - rather than adding what looks sort of like redundant testing to existing test cases?
>>>>>
>>>>> I am not sure how to construct corrupted CU Index manually. Would Yaml2Obj be able to do it?
>>>>
>>>> I'm not sure - I don't think so. Might just have to be raw assembly - check the other symbolizer dwp testing? I think it's probably hand-crafted assembly? (maybe llvm-dwp testing that merges existing dwp files has some hand crafted dwp files too you could be inspired by)
>>>>
>>>> But maybe Yaml2Obj could help - I'm just not very familiar with this.
>>>>
>>>>> For other tests I think it's ads testing coverage to make sure we are parsing cu/tu indexes correctly in various DWARF versions and debug types enablements combinations.
>>>>
>>>> I'd rather keep things a bit narrower - demonstrate that the index is computed correctly, and assume that everything else works correctly once the index is correct.
>>>
>>> So...
>>> Good news is that I got inspired and created a hand written assembly with invalid index.
>>> Bad news is that this version of the patch relies on overflow behavior. See uint32_t TruncOffset.
>>
>> UB when converting a uint64_t into a uint32_t? Hmm, I didn't think that was undefined - figured that'd wrap around with well defined behavior. Maybe I'm misunderstanding - what's the particular UB you're referring to?
>
> Sorry was a bit vague with the terms. When I said "overflow behavior" I didn't mean it's UB, but was referring to "wrap around behavior" which is defined.
>
>>> So just having one incorrect offset doesn't do anything.
>>
>> Yeah, I'm still missing a step here. But I guess since we're only walking the offsets and looking for wraparound, this only has an effect if there's a genuine overflow, which would require a genuinely very large dwp file, which is infeasible to checkin as a test?
>
> Right exactly. We are iterating over headers and computing TruncOffset and don't hit the bad case until we wrap around (.debug_info.dwo over 4GB). So I don't see a way to create a test to trigger this unless we have a very large dwp file.

*nod* fair enough. Any chance of hand writing something that uses small assembly, but produces large enough output that it hits the overflow (using `.zero` or the like) - we wouldn't want to run it (don't want to produce multi-GB output files in test execution) but maybe comment it as a manual test, or at least have mentioned it/shown it in this review.

If you could write up something like that, copy/paste manually running it to show that the patch makes a difference and include the assembly in this review, I guess that'll have to be adequate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137882