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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Generally looks okay to me. I've made a few inline comments.

This approach works for .debug_ranges, because we are working with pairs of addresses, but we have to be more careful with other DWARF sections, because on some architectures, address zero is a valid address and should not be used, and we cannot simply set the size to zero, because we don't know where that is (if anywhere at all). gdb recognizes zero as special, if the architecture does not allow zero addresses, but otherwise it is not treated as special, and so we end up with (potentially) overlapping debug info entries which can cause problems with the debugger. Similarly, .debug_aranges uses an address and a fixed size, which we have to update correctly. There's a related discussion on the LLDB mailing list I started about handling GC'ed sections in the debugger: http://www.mail-archive.com/lldb-dev@lists.llvm.org/msg04140.html. The general agreement is that we should use a special non-zero address. I would suggest -1 (i.e. 0xFFFFFFFF in 32-bit DWARF addresses and 0xFFFFFFFFFFFFFFFF in 64-bit DWARF addresses), but we should have that discussion on the mailing lists.



================
Comment at: ELF/InputSection.cpp:487
 
+static bool isDiscardedSection(SymbolBody &Body) {
+  DefinedRegular *D = dyn_cast<DefinedRegular>(&Body);
----------------
Should be called isInDiscardedSection, because it is working with a symbol not a section. "isDiscardedSection" suggests to me that it is asking if a section has been discarded or not, and so would take an InputSection or section index.


================
Comment at: ELF/InputSection.cpp:510
+
+  bool IsDebugRanges = Name == ".debug_ranges";
   for (const RelTy &Rel : Rels) {
----------------
Is there anywhere else that we check section names? If so, I'd personally prefer if we set some flag on the section early rather than doing string comparisons on every input section to test the name multiple times.


================
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.
----------------
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."


https://reviews.llvm.org/D31464