[PATCH] D58194: [DebugInfo] add SectionedAddress to DebugInfo interfaces.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Ok. I debugged this a little and think that:

1. LLD code change LGTM.
2. I find the LLD test case potentially useful because I did not find any test checking exactly this situation. Given that LLD has mentioned "TODOs" in the code that we might want to remove, it would be good to have such a test for the future possible changes probably.
3. It feels that the test can be reduced and might need some cleanup (I left a few inline comments).
4. Since without assigning the section index, we would have few existent LLD tests failures (and also since this test pass without any code changes atm), it should be fine to go without any additional test for LLD in this patch.

To summarize: I suggest to proceed with this patch without the LLD test and post it on a different review as an independent improvement.



================
Comment at: lld/test/ELF/debug-line-obj.s:5
+
+# Check we do not crash and able to report the source location.
+
----------------
I do not think you need to mention "crash" here.
Seems you used debug-line-str.s test case as a base and the comment is coming from there.
That test was committed with the fix that actually fixed a crash and that is why it is was mentioned.

I would just say here that this test checks that we report locations correctly when we have the line number
information in .debug_line that describes lines from different sections created because of using of -ffunction-sections flag.
Or something like that :)



================
Comment at: lld/test/ELF/debug-line-obj.s:238
+.Ldebug_rnglist_table_end0:
+	.section	.debug_macinfo,"", at progbits
+	.byte	0                       # End Of Macro List Mark
----------------
You should not need `.debug_macinfo` section.


================
Comment at: lld/test/ELF/debug-line-obj.s:251
+
+	.ident	"clang version 9.0.0 (https://github.com/llvm/llvm-project.git 1081413baa15d9faa1092706468eedddddd8bc35)"
+	.section	".note.GNU-stack","", at progbits
----------------
I think you can remove .ident and the `.note.GNU-stack` section below.


================
Comment at: llvm/test/tools/llvm-objdump/function-sections-line-numbers.test:1
+# RUN: clang -ffunction-sections -gdwarf-5 -O -c %p/Inputs/function-sections-line-numbers.cpp -o %t.o 
+# RUN: llvm-objdump -disassemble -line-numbers -r -s -section-headers -t %t.o | FileCheck %s
----------------
I did not find any other llvm-objdump test that would use clang.
You should probably use llvm-mc I think.


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

https://reviews.llvm.org/D58194