[PATCH] D95927: DebugInfo/Symbolize: Retrieve filename from the preceding STT_FILE for .symtab symbolization

Tue Feb 9 01:20:51 PST 2021

jhenderson added a comment.

In D95927#2549451 <https://reviews.llvm.org/D95927#2549451>, @MaskRay wrote:

> In D95927#2548006 <https://reviews.llvm.org/D95927#2548006>, @jhenderson wrote:
>
>> There are some pre-merge bot failures looking at this patch, although I guess some of them might be a patch ordering issue.
>>
>> Do we have test cases for the following negative aspects where the symbol isn't used?
>>
>> 1. The symbol name can't be looked up due to a corrupt st_name value.
>> 2. There are no STT_FILE symbols before the local symbol in question (possibly there may be some after).
>> 3. The symbol is a global symbol, with an STT_FILE symbol before it.
>> 4. The symbol has a corresponding STT_FILE symbol, but the name from .debug_line can be found.
>
> The current `--use-symbol-table=true` (which allows .symtab information to override debug info) has no test coverage. The previous patch and this patch added more coverage in this area.
>
> There are some interesting cases but I think the coverage can be added separately.

All of the cases I highlighted are new cases introduced by this patch, since the STT_FILE symbols were not used to look up file names previously. Whether or not the rest of `--use-symbol-table=true` functionality is tested is rather irrelevant to this, I think. We shouldn't be creating additional technical debt by adding more areas that aren't tested properly.

> Making sure .file & debug info have consistent information is the responsibility of other components (DebugInfo/AsmPrinter/ELFObjectWriter/etc). The additional tests can check what we behave when inconsistent information is present.

The Symbolizer library needs testing for consumption of both consistent and inconsistent information. I don't think the responsibility of ensuring this information is consistent is relevant to that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95927