[PATCH] D79165: [DebugInfo] - DWARFDebugFrame: do not call abort() on errors.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the patch. Error reporting infrastructure in some components of LLVM is shaky. The merit of report_fatal_error is just that it is easy. As Eli Friedman mentioned, "you don't have to worry about formatting the diagnostic, or the implications of what happens after the error is triggered, or a location for the diagnostic." We do need to use more appropriate error reporting mechanism where we can.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:532
+            Entries.back()->cfis().parse(Data, &Offset, EndStructureOffset))
+      return std::move(E);
 
----------------
The return type has been changed to `Error`. `return std::move(E)` should be written as `return E` to avoid clang `warning: moving a local object in a return statement prevents
 copy elision [-Wpessimizing-move]`


================
Comment at: llvm/test/DebugInfo/X86/eh-frame-cie-id.s:1
-# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
-# RUN:   not --crash llvm-dwarfdump -debug-frame - 2>&1 | \
-# RUN:   FileCheck %s
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t
+# RUN: not llvm-dwarfdump -debug-frame %t 2>&1 | FileCheck %s
----------------
grimar wrote:
> ikudrin wrote:
> > Why do you need to keep that temporary file?
> Having temporarily files helps to debug tests when they fail.
I tend to agree with the temporary file. llvm-lit requires me to change the current working directory to somewhere deep in bindir and I feel it cumbersome, so I started to use a simplified version of llvm-lit to run simple tests during debugging.

```
#!/usr/bin/env python3
import os, sys

os.environ["PATH"] = os.path.expanduser('/tmp/Debug/bin') + os.pathsep + os.environ['PATH']
fname = sys.argv[1]
last = ''

with open(sys.argv[1]) as f:
    for line in f.readlines():
        line = line.strip()
        i = line.find('RUN: ')
        if i < 0:
            last = ''
            continue
        line = line[i+5:]
        if line[-1] == '\\':
            last += line[:-1]
            continue
        line = last + line
        last = ''

        line = line.replace('%s', fname)
        line = line.replace('%S', '.')
        line = line.replace('%p', '.')
        line = line.replace('%t', 'a')
        line = line.replace('%clang_cc1', 'clang -cc1 -internal-isystem /tmp/Debug/lib/clang/11.0.0/include -nostdsysteminc')
        print(line)
        os.system(line)
```


================
Comment at: llvm/test/DebugInfo/X86/eh-frame-cie-id.s:4
 
-# CHECK: Parsing FDE data at 0 failed due to missing CIE
+# CHECK: Parsing FDE data at 0x0 failed due to missing CIE
 
----------------
grimar wrote:
> ikudrin wrote:
> > Prefixing offsets with `0x` should be probably extracted to a separate patch.
> This error message is just broken without `0x` as it prints values in hex, but you can't know it.
> We often adjust/improve messages when doing similar changes. E.g. add error locations or an additional context.
> This is the only LLVM test which is affected anyways.
Since the diagnostic line is touched and this is the only test, I think it is fine to adjust it in this patch.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/unwind.test:220
+## Check we report an error when the tool is unable to parse .eh_frame section.
+## Previously it also suggested to send an error report to LLVM.
+# RUN: yaml2obj --docnum=2 %s -o %t2.exe
----------------
We don't need to mention the previous behavior.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/unwind.test:237
+    Type: SHT_X86_64_UNWIND
+## The content is generated from the following code. It has no CIE.
+## See the DebugInfoX86/eh-frame-cie-id.s test case for more history.
----------------
Perhaps an assembly test is better here? It will require `REQUIRES: x86-registered-target`.

(You may be interested in this thread: 
http://lists.llvm.org/pipermail/llvm-dev/2020-April/141203.html "[yaml2obj] GSoC-20: Add DWARF support to yaml2obj" also see the same thread on 2020-March


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

https://reviews.llvm.org/D79165