[PATCH] D82173: [DWARFYAML][debug_info] Use 'AbbrCode' to index the abbreviation.

Fri Jun 19 08:05:27 PDT 2020

grimar added inline comments.

================
Comment at: llvm/lib/ObjectYAML/DWARFVisitor.cpp:59
+      // TODO: Add error handling and test this error.
+      assert(AbbrCode <= DebugInfo.AbbrevDecls.size());
+      auto &Abbrev = DebugInfo.AbbrevDecls[AbbrCode - 1];
----------------
jhenderson wrote:
> Higuoxing wrote:
> > grimar wrote:
> > > Doesn't seem this assert is helpful. `AbbrevDecls` is `std::vector<Abbrev>`,
> > > Most (or all) of STL implementations check the out of bounds access internally I think.
> > > 
> > > Probably you should add a test case and do the following for now:
> > > 
> > > ```
> > > if (AbbrCode > DebugInfo.AbbrevDecls.size())
> > >   report_fatal_error(...)
> > > ```
> > Sorry, I haven't tested a `report_fatal_error()` before. What will be checked? The dumped stack trace? or something else? Can I leave a "TODO" here and let this function return `Error` in a follow-up patch?
> Typically, you wouldn't test a `report_fatal_error` as it shouldn't be possible to reach that bit of code. It's like an assert, but works without asserts being enabled too.
> What will be checked? The dumped stack trace? or something else?

Just the fact of crash of the test case. In the follow-up you could fix the crash then.
It should be possible to check that crash with `--crash` option for `not`:

// RUN: llvm-mc -filetype=obj -triple aarch64-windows %s -o - \
// RUN:   | not --crash llvm-readobj --unwind - 2>&1 | FileCheck %s

I do not mind leaving the test case for a follow-up. It was just important not to use an assert here.
`report_fatal_error` calls `abort()`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82173