[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for emitting multiple abbrev tables.

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:34
+private:
+  Data &DWARF;
+  std::map<uint64_t, uint64_t> AbbrevID2Index;
----------------
Would it make any sense to merge the `DWARFYAML::Data` class and `DWARFYAML::DWARFState` class at all?


================
Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:35
+  Data &DWARF;
+  std::map<uint64_t, uint64_t> AbbrevID2Index;
+
----------------
Can you use `std::unordered_map` here? Do we need deterministic ordering? Might also be worth naming it something like `AbbrevTableID2Index` to avoid confusing with the `AbbrevCode` values.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:92
+  for (uint64_t I = 0; I < DI.DebugAbbrev.size(); ++I) {
+    // If the abbrev table's ID isn't specified, we use the index as its ID.
+    uint64_t ID = I;
----------------
Maybe to avoid weird errors, it might make sense to disallow mixing the two methods, i.e. if one table has an explicit ID, the others all must do too. What do you think? It may not be worth it if the code is more complex, I don't know.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:105
+    }
+    AbbrevID2Index.insert({ID, I});
+  }
----------------
Could you use the return value of `insert` to identify whether the key already exists in the map? That way, you don't need the explicit count call.


================
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(
----------------
It might make more sense to do this work in the caller of this function, and to maintain this function's interface.


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:271
+  // There might be DWARF sections and they might be interlinked. We use
+  // DWARFState to get them interlinked.
+  Optional<DWARFYAML::DWARFState> DWARFState;
----------------



================
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.
----------------
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.


================
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
----------------
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?


================
Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:873-884
+      - sectname:  __debug_abbrev
+        segname:   __DWARF
+        addr:      0x00
+        size:      24
+        offset:    528
+        align:     0
+        reloff:    0x00000000
----------------
Can you omit the abbrev section entirely?


================
Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:906-911
+## e) Test that yaml2obj emits an error message when multiple abbrev tables are assigned
+## the same ID.
+
+# RUN: not yaml2obj --docnum=5 %s 2>&1 | FileCheck %s --check-prefix=ID-COLLISION
+
+# ID-COLLISION: yaml2obj: error: the ID (1) of abbrev table with index 1 has been used
----------------
This feels like it belongs in a debug_abbrev dedicated test?


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:924-925
+
+## o) Test that yaml2obj emits an error message when multiple abbrev tables are assigned
+## the same ID.
+
----------------
Same as Mach-O, this really is a debug_abbrev test, not a debug_info test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83116