[PATCH] D59553: [WIP][LLD][ELF][DebugInfo] Use predefined value to mark debug address ranges pointing to deleted code.

Thu Jun 11 01:14:59 PDT 2020

jhenderson added a comment.

FWIW, I wonder whether the test case would be better written with YAML rather than assembly input. It would allow for more explicitly specification of what relocations are emitted, and would allow reusing the same yaml blob using the `-D` option for the differences between the 32-bit and 64-bit cases, relocation types etc.

================
Comment at: lld/ELF/InputSection.cpp:845-846

+// This function returns special value which is used to indicate reference
+// from debug section into the deleted code.
+uint64_t InputSection::getRelocValueForDeletedCode(RelType type,
----------------
returns a special value
indicate a reference from a debug

================
Comment at: lld/ELF/InputSection.cpp:852-853
+
+  if (config->is64 && config->emachine == EM_X86_64 && type == R_X86_64_32)
+    res &= 0x00000000ffffffffULL;
+
----------------
I might well have missed something, but why this special casing specifically for X86_64? Do other 64-bit targets not have the same issue (I'm assuming the masking is required to prevent some sort of "too big to fit" error from the relocation processing).

It also looks like some of this if statement is untested (specifically the bit about EM_X86_64).

================
Comment at: lld/ELF/InputSection.cpp:870
+  bool isDebug = isDebugSection(*this);
+  StringRef nameWOPrefix = name.substr(name.find_first_not_of(".z"));
+  bool isDebugLocOrRanges =
----------------
I struggled to interpret this variable name without looking at how it is used. Perhaps `noPrefixName` might be better.

================
Comment at: lld/ELF/InputSection.cpp:929-930
+    // If the relocation from .debug* section points to a deleted section,
+    // e.g. due to --gc-sections, then it is neccessary to resolve such
+    // relocation to the special value indicating deleted code.
+    else
----------------
to resolve such relocation -> to resolve it

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:4
+# RUN: echo '.section .text,"a"; .byte 0;  .section .debug_info,"", at progbits; .quad .text' | \
+# RUN:   llvm-mc -filetype=obj -arch=x86-64 - -o %t
+# RUN: ld.lld %t -o %t2 --gc-sections
----------------
jhenderson wrote:
> I don't know what the common format in LLD tests is off the top of my head, but I find it easier if the '|' is on the following line:
> 
> ```
> # RUN: echo .... \
> # RUN:   | llvm-mc ...
> ```
> I don't know what the common format in LLD tests is off the top of my head, but I find it easier if the '|' is on the following line:
Hehe, amusing self-note looking back at this comment - I nowadays prefer the pattern the other way around, namely end one line with `| \` and start the next line with a couple of extra spaces of indentation. I'm happy whichever way you wish to do it.

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:3
+
+# RUN: echo '.section .text,"a"; .byte 0;  .section .debug_foo,"", at progbits; .quad .text; .section .debug_loc,"", at progbits; .quad .text' \
+# RUN:   | llvm-mc --filetype=obj -triple=x86_64-unknown-linux - -o %t
----------------
Is there a reason you are echoing this content, rather than having it written in the test file like most tests (I assume it's the need for two different blocks of assembly)? If there is, then the test file should end with .test, rather than .s since this file currently contains no assembly.

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:4
+# RUN: echo '.section .text,"a"; .byte 0;  .section .debug_foo,"", at progbits; .quad .text; .section .debug_loc,"", at progbits; .quad .text' \
+# RUN:   | llvm-mc --filetype=obj -triple=x86_64-unknown-linux - -o %t
+# RUN: ld.lld %t -o %t2 --gc-sections
----------------
Use different file names for each of the test cases, e.g. `%t.x64` and `%t.i386`

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:18
+
+# When --gc-sections is used, the linker deletes unused text sections,
+# but their debug data is left in the binary. That debug data could have relocations
----------------
I'd move this whole comment to the top of the file to show what is being tested. I'd also change the `debuginfo` part of the test name to `debug-data` (i.e. `gc-sections-debug-data.s`).

Actually, this isn't specific to `--gc-sections` right? The same code applies for discarded COMDATs, if I'm not mistaken? So should that be tested too?

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:22
+# with other address ranges for correct debug data. To prevent addresses clashing
+# it is neccessary to resolve such relocations to an predefined value. 
+# That test checks that relocations from the .debug_foo
----------------
to an predefined -> to a predefined

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:23
+# it is neccessary to resolve such relocations to an predefined value. 
+# That test checks that relocations from the .debug_foo
+# section pointing to a deleted .text section would be resolved
----------------
That test -> This test

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:24
+# That test checks that relocations from the .debug_foo
+# section pointing to a deleted .text section would be resolved
+# to 0xffffffffffffffff. And relocations from the .debug_loc section
----------------
would be -> are

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:25
+# section pointing to a deleted .text section would be resolved
+# to 0xffffffffffffffff. And relocations from the .debug_loc section
+# pointing to a deleted .text section would be resolved
----------------
. And -> and

We should explicitly test .debug_ranges as well as .debug_loc, since they are both special cased.

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:26
+# to 0xffffffffffffffff. And relocations from the .debug_loc section
+# pointing to a deleted .text section would be resolved
+# to 0xfffffffffffffffe.
----------------
would be -> are

================
Comment at: lld/test/ELF/gc-sections-debuginfo.s:64-65
+# CHECK-NEXT: )
+
+
----------------
Nit: there's still a double blank line at EOF here that should be deleted.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59553