[lld] [ELF] Change .debug_names tombstone value to UINT32_MAX/UINT64_MAX (PR #74686)
Alexander Yermolovich via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 18 15:23:21 PST 2023
ayermolo wrote:
> > [reviews.llvm.org/D84825](https://reviews.llvm.org/D84825) has a TODO that we should switch the
> > tombstone value for most `.debug_*` sections to UINT64_MAX.
>
> Your input has been valuable in guiding this enhancement. I truly appreciate the .debug_names work you initiated and put in this linker patch. Now, the code here isn't too complex but the two implementations have some nuances, though not every aspect is tested.
>
> 1. What shall we do for a non-R_ABS tombsone value? [[LLD] Tombstone LocalTU entry in .debug_names #70701](https://github.com/llvm/llvm-project/pull/70701) will apply the tombstone value and [[ELF] Change .debug_names tombstone value to UINT32_MAX/UINT64_MAX #74686](https://github.com/llvm/llvm-project/pull/74686) won't.
>
> I feel that we can reject it. My initial `-z dead-reloc-in-nonalloc=` patch (which I do want to support non-symbolic R_ABS relocations) did not consider it as it would complicate the code. Now we have to handle it. I feel that `if (tombstone && (expr == R_ABS || expr == R_DTPREL)) {` as in this patch is better.
>
> 2. What shall we do for a short R_ABS tombstone value?
>
> #70701 uses `if (!tombstone && type == target.symbolicRel)`. With some thoughts, I believe we should use `if (emachine == EM_X86_64 && type == R_X86_64_32)` to make it clear it's x86-64 specific to differentiate 32-bit signed/unsigned relocation types. If we eventually make `relocateNonAlloc` target-specific (as I did for `relocateAlloc`), the R_X86_64_32 code can be make X86_64.cpp specific.
>
> Considering the necessity for certain improvements, I've put efforts to write a detailed description and tests (including the test enhancement to `debug-dead-reloc.s` and a 32-bit test (which I omitted for my initial `-z dead-reloc-in-nonalloc=` patch)) that align better with the existing convention of these modified tests. For example, I think it is important to call out the TODO from reviews.llvm.org/D84825 in the description.
>
> I'd be more than happy to include you as a co-author acknowledging your contribution. If you'd prefer any specific recognition or involvement moving forward, please let me know. Your input has been valuable in guiding this enhancement.
Thank you for explanation. I think it would be valuable for me, and for the health of the project, if next time we could continue discussion in the original patch I think it would benefit everyone. :)
https://github.com/llvm/llvm-project/pull/74686
More information about the llvm-commits
mailing list