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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:419
+            return createStringError(
+                errc::illegal_byte_sequence,
+                "unknown augmentation character in entry at 0x%" PRIx64,
----------------
grimar wrote:
> grimar wrote:
> > jhenderson wrote:
> > > grimar wrote:
> > > > jhenderson wrote:
> > > > > 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.
> > > > This file already use `illegal_byte_sequence` (line 50, 65). It is also used widely in LLVM to report invalid non-text data.
> > > 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.
> > 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
> > 
> > 
> 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".
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).


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

https://reviews.llvm.org/D79165