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

[Xing GUO via Phabricator via llvm-commits]

Higuoxing 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];
----------------
grimar wrote:
> 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()`.
Thanks! I added one more test case for it :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82173