[PATCH] D31464: [ELF] - Stop producing broken entries in .debug_ranges section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

LGTM, with the inline comments. One of @ruiu or @rafael should probably still approve it though.



================
Comment at: ELF/InputSection.cpp:545
 
+static bool isInDiscardedSection(SymbolBody &Body) {
+  DefinedRegular *D = dyn_cast<DefinedRegular>(&Body);
----------------
I think this can be a const reference.


================
Comment at: ELF/InputSection.cpp:533-538
+    // If address range list contains entry that belongs to discarded
+    // section, we should perform a fix up. We can not write zero as start and
+    // end address because that would mean unexpected end of range. So we put a
+    // placeholder value equal to 1, that is correct to do because a range list
+    // entry whose begining and end adress are equal has no effect, because size
+    // of range is zero in such case.
----------------
grimar wrote:
> jhenderson wrote:
> > A few grammar nits and minor improvements to this comment. I suggest the following:
> > 
> > "If an address range list contains an entry that belongs to a discarded section, we should perform a fix-up. We cannot write zero as the start and end address because such a pair is an end of range list marker in the .debug_ranges section. We put a placeholder value equal to 1, which is fine because a range list entry whose beginning and end address are equal has no effect, because the size of the range is zero in such case."
> Fixed. thanks !
Sorry, one more grammar nit I missed - the last bit should say "in such a case."


================
Comment at: test/ELF/debug-ranges-reloc.s:4
+# RUN: ld.lld %t1.o -o %t -gc-sections -e main
+# RUN: llvm-dwarfdump -debug-dump=all %t | FileCheck %s
+
----------------
grimar wrote:
> jhenderson wrote:
> > We only need the -debug-dump=ranges, since we are only interested in the .debug_ranges output.
> It does not work. Looks llvm-dwarfdump has an issue here (it does not dump anything with =ranges only for this testcase), I'll try to investigate and may be prepare patch for it separatelly. I suggest to go with =all for now.
I just ran into this problem as well! Fixing dwarfdump would be great.


https://reviews.llvm.org/D31464