[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for referencing different abbrev tables.

[Xing GUO via Phabricator via lldb-commits]

Higuoxing added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:306-313
+  Optional<uint64_t> AbbrevTableIndex =
+      DS.getAbbrevTableIndexByID(AbbrevTableID);
+  if (!AbbrevTableIndex)
+    return createStringError(errc::invalid_argument,
+                             "cannot find abbrev table whose ID is " +
+                                 utostr(AbbrevTableID));
+  ArrayRef<DWARFYAML::Abbrev> AbbrevDecls(
----------------
jhenderson wrote:
> It might make more sense to do this work in the caller of this function, and to maintain this function's interface.
This is discussed in [D86194](https://reviews.llvm.org/D86194#inline-796173)


================
Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:679-681
+## c) Test that yaml2obj is able to generate compilation units according to the
+## associated abbrev table that is referenced by the 'AbbrevTableID' and obj2yaml
+## is able to convert it back.
----------------
jhenderson wrote:
> Is there any particular reason you don't have something equivalent to this in the ELF testing, to show you can have tables out-of-order etc.
Added in test/tools/yaml2obj/ELF/DWARF/debug-info.yaml (o)


================
Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:711
+# MULTI-TABLES-NEXT:               Form:      DW_FORM_udata
+# MULTI-TABLES-NEXT:   debug_info:
+# MULTI-TABLES-NEXT:     - Length:        0x000000000000000C
----------------
jhenderson wrote:
> Something's not right here - the YAML below has four debug_info tables, but only three have check lines. What happened to the one that should be referencing abbrev table 2?
Sorry, the length and offsets are calculated by myself, and the __debug_info section's length is miscalculated to be 60, which should be 67. I've corrected it.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:658
+
+# NOTABLE: yaml2obj: error: abbrev table index must be less than or equal to the number of abbrev tables
+
----------------
jhenderson wrote:
> Perhaps worth including the index and table count values in this error message to make it easier for people to identify the problem.
I've included the abbrev table ID and compilation unit count values in test case (n). Can we prettify the error message in test case (i) in a follow-up patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83116