Page MenuHomePhabricator

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

Authored by grimar on Apr 30 2020, 3:22 AM.

Details

Summary

Imagine we have a broken .eh_frame.
Below is a possible sample output of llvm-readelf:

...
    entry 2 {
      initial_location: 0x10f5
      address: 0x2080
    }
  }
}
.eh_frame section at offset 0x2028 address 0x2028:
LLVM ERROR: Parsing entry instructions at 0 failed
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.  Program arguments: /home/umb/LLVM/LLVM/llvm-project/build/bin/llvm-readelf -a 1
 #0 0x000055f4a2ff5a1a llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/umb/LLVM/LLVM/llvm-project/build/bin/llvm-readelf+0x2b9a1a)
...
#15 0x00007fdae5dc209b __libc_start_main /build/glibc-B9XfQf/glibc-2.28/csu/../csu/libc-start.c:342:3
#16 0x000055f4a2db746a _start (/home/umb/LLVM/LLVM/llvm-project/build/bin/llvm-readelf+0x7b46a)
Aborted

I.e. it calls abort(), suggests to submit a bug report and exits with the code 134.
This patch changes the logic to propagate errors to callers.
This fixes the behavior for llvm-dwarfdump, llvm-readobj and other possible tools.

Diff Detail

Event Timeline

grimar created this revision.Apr 30 2020, 3:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar updated this revision to Diff 261178.Apr 30 2020, 3:25 AM
  • Fix comment.
grimar updated this revision to Diff 261179.Apr 30 2020, 3:27 AM
  • And one more.

Overall looks good. You may want to add a note to PR45105.

llvm/test/DebugInfo/X86/eh-frame-cie-id.s
1–2

Why do you need to keep that temporary file?

4

Prefixing offsets with 0x should be probably extracted to a separate patch.

grimar marked 2 inline comments as done.Apr 30 2020, 7:09 AM

You may want to add a note to PR45105.

Ah. I didn't see it, thanks! I faced this crash independently using another broken object.

llvm/test/DebugInfo/X86/eh-frame-cie-id.s
1–2

Having temporarily files helps to debug tests when they fail.

4

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.

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.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
536

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]`

llvm/test/DebugInfo/X86/eh-frame-cie-id.s
1–2

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)
4

Since the diagnostic line is touched and this is the only test, I think it is fine to adjust it in this patch.

llvm/test/tools/llvm-readobj/ELF/unwind.test
220

We don't need to mention the previous behavior.

237

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

grimar updated this revision to Diff 261440.May 1 2020, 1:16 AM
grimar marked 9 inline comments as done.
  • Addressed review comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
536

Ah. I was under impression that return E instead of std::move(E) won't compile, because it usually doesn't (when the function returns Expected<>, what is very common), but I've missed we return an Error here. Thanks!

llvm/test/tools/llvm-readobj/ELF/unwind.test
237

I expect one day we will be able to rewrite it to yaml. At least adding a support for .eh_frame does not feel too hard to me, though I've not tried and may miss something. And this sample is little, so I'd leave it as YAML.

jhenderson added inline comments.May 1 2020, 2:03 AM
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
819–823

What do you think about fixing the naming of this variable to use UpperCamelCase here? (I see there might be some concerns, hence the question).

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
418–421

createStringError I think would be better?

createStringError would also allow this to change back to PRIx64, if I'm not mistaken.

Also, inconvertibleErrorCode() should probably be replaced with something else, probably invalid_argument.

427–430

Ditto to above.

445–448

Ditto to above.

462–466

Ditto to above.

489–493

Ditto to above.

519–523

Ditto to above.

539–544

Ditto to above.

MaskRay added inline comments.May 1 2020, 4:25 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
418–421

Maybe we can de-capitalize the diagnostic by the way.

createStringError(errc::invalid_argument, "unknown augmentation character in entry at 0x" PRIx64, ...)

grimar updated this revision to Diff 263434.Tue, May 12, 7:33 AM
grimar marked 10 inline comments as done.
  • Addressed review comments.
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
819–823

Ok, since it was introduced by this patch.

jhenderson added inline comments.Wed, May 13, 1:30 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
419

Don't use illegal_byte_sequence - although it's not clear from the name, this is for Unicode encoding errors, so isn't the right error code for other invalid byte sequences. I've tended to use invalid_argument in similar situations.

grimar marked an inline comment as done.Wed, May 13, 2:07 AM
grimar added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
419

This file already use illegal_byte_sequence (line 50, 65). It is also used widely in LLVM to report invalid non-text data.

jhenderson added inline comments.Wed, May 13, 2:58 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
419

Just because it's already used widely incorrectly doesn't mean we should continue to do so - I'm pretty confident that people are all making the same, perfectly understandable, mistake.

grimar marked an inline comment as done.Wed, May 13, 3:23 AM
grimar added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
419

Where it is said that illegal_byte_sequence is about unicode?

MSVS defines it as "illegal_byte_sequence = 42, // EILSEQ" (xerrc.h)

POSIX seems descrives it as "[EILSEQ] Illegal byte sequence."
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

C95 says the same:
https://en.cppreference.com/w/c/error/errno_macros

grimar marked an inline comment as done.Wed, May 13, 3:38 AM
grimar added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
419

Seems that EILSEQ is used by functions like fputwc to report errors about wide characters. But I haven't found that it can't be used for other cases. And "Illegal byte sequence" description fits here much better than "Invalid argument".

jhenderson added inline comments.Wed, May 13, 4:41 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
419

You're right that the standard doesn't explicitly say it is for multibyte/UTF-8 etc, but every reference I found to it outside of LLVM is to do with such instances, so I think it can be reasonably inferred that that is the expected usage (I thought I ran into something at some point a couple of years ago where the illegal_byte_sequence usage actually caused a problem in LLVM because of this, although I haven't been able to identify where yet). invalid_argument is perfectly correct - the character being used here is not a valid input character (think of this piece of code as the augmentation string constructor).

grimar updated this revision to Diff 263709.Wed, May 13, 7:16 AM
  • Use invalid_argument.
jhenderson accepted this revision.Thu, May 14, 1:26 AM

LGTM, but please give a chance for the other reviewers to look further too.

This revision is now accepted and ready to land.Thu, May 14, 1:26 AM
MaskRay accepted this revision.Thu, May 14, 8:55 AM
MaskRay added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
819–823

auto because the type is obvious from make_unique

834

auto

This revision was automatically updated to reflect the committed changes.
grimar marked 2 inline comments as done.