[PATCH] D82886: [DebugInfo] Fix a possible crash when reading a malformed .debug_*lists section.

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-rnglists-format-mix.s:1
+## The test checks that llvm-dwarfdump with enabled assertions can handle
+## a malformed input file that contains debugging info sections in different
----------------
jhenderson wrote:
> I think you can get rid of the "with enabled assertions" comment here. The test also shows that llvm-dwarfdump without assertions doesn't crash!
> 
> As this is testing the library code, rather than something to do with llvm-dwarfdump specifically, perhaps you can just drop `dwarfdump-` from the test name.
> I think you can get rid of the "with enabled assertions" comment here. The test also shows that llvm-dwarfdump without assertions doesn't crash!
I'll update the wording, thanks!

> As this is testing the library code, rather than something to do with llvm-dwarfdump specifically, perhaps you can just drop dwarfdump- from the test name.
We have a group of tests here with assembly inputs that check various aspects of the library. I wanted the new test to be in the same group, next to other tests which also start with "dwarfdump-rnglists".


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-rnglists-format-mix.s:5-7
+# RUN: llvm-mc -triple x86_64 %s -filetype=obj -o - \
+# RUN:   | not llvm-dwarfdump -debug-info - 2>&1 \
+# RUN:   | FileCheck %s
----------------
jhenderson wrote:
> Two thoughts, and not asking you to change anything if you prefer the current approach:
> 
> 1) I'm not a fan of piping llvm-mc output into llvm-dwarfdump directly, because it makes it harder to debug the test if there's a problem, since I then have to change this line to generate the intermediate object etc.
> 2) I've come to the conclusion that I personally prefer having the pipe operator at the end of the previous line, since it clearly shows that the next line is new command, and that you don't need to look at it to find more arguments for the command on the first line, i.e:
> ```
> # RUN: llvm-mc -triple x86_64 %s -filetype=obj -o - | \
> ## Indentation shows this is a continuation; llvm-dwarfdump shows it's a new executable being called
> # RUN:   not llvm-dwarfdump -debug-info - 2>&1 | \
> # RUN:   FileCheck %s
> ```
Will do. Thanks.


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-rnglists-format-mix.s:6
+# RUN: llvm-mc -triple x86_64 %s -filetype=obj -o - \
+# RUN:   | not llvm-dwarfdump -debug-info - 2>&1 \
+# RUN:   | FileCheck %s
----------------
jhenderson wrote:
> Do you actually need to dump the debug info, or could you just dump the .debug_rnglists section explicitly? That would allow you to simplify your test input accordingly.
The test was based on the sample I run into when worked on producing DWARF64 debugging info. Probably you are right, the test may be simplified to check only the specific issue.


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

https://reviews.llvm.org/D82886