[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:
> 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?


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:617-625
+      AbbrOffset: 0x1234
+      AddrSize:   4
+      Entries:
+        ## Test that yaml2obj uses the same abbreviation table to generate values
+        ## across different compilation units.
+        - AbbrCode: 2
+          Values:
----------------
jhenderson wrote:
> There's a bit of tension here between the AbbrOffset and the behaviour. Since the AbbrOffset should point to the start of the table containing the corresponding abbrevs, it's a little unclear what the behaviour should be if the offset is nonsensical. It also prevents us having multiple abbrev tables as it stands.
> 
> I think it's okay to add this test case for now, but I might suggest a change for the future, to split the abbrev section into distinct tables which can be referenced by some ID from the .debug_info. Different .debug_info tables can refer to the same or different abbrev tables that way. The offset would be auto-calculated to match, but could be overridden to a different value (the ID could still be used to actually identify how to write the values).
> I think it's okay to add this test case for now, but I might suggest a change for the future, to split the abbrev section into distinct tables which can be referenced by some ID from the .debug_info. Different .debug_info tables can refer to the same or different abbrev tables that way. The offset would be auto-calculated to match, but could be overridden to a different value (the ID could still be used to actually identify how to write the values).

Yeah, the current implementation of the .debug_abbrev section doesn't support multiple tables. I deleted this test. I will it once generating multiple abbreviation tables is supported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82173