jhenderson added a comment. In D83116#2130019 <https://reviews.llvm.org/D83116#2130019>, @labath wrote:
> What would you say if, instead of `AbbrevTableIndex`, we had a field like > `AbbrevTableID`. The main difference would be that this "ID" field can be > explicitly specified on the Abbrev table, and it does not have to be a > sequentially increasing number (though it could of course be that by default). > > The thing I'm trying to achieve is to make the yaml more robust against > modifications/simplifications. E.g., it would be nice if deleting an abbrev > table does not make all compile units suddenly refer to different tables. If > the ids were present explicitly, compile units would be unaffected by this, > and one would get an explicit error message if there was a compile unit left > which still referred to the deleted abbrev table. > > (This is one of the aspects where an assembler is better than yaml, as > symbolic labels in asm have these properties.) +1 to this idea. By default it could of course be an incremental index, when emitting, but in yaml2obj, I see no reason for it to be tied in that way necessarily. ================ Comment at: lldb/unittests/Symbol/Inputs/inlined-functions.yaml:348 + Version: 4 + AbbrevTableIndex: 1 + AbbrOffset: 0 ---------------- `AbbrevTableIndex` should be an optional value. I think by default it should pick the first table (or possibly should pick the Nth table, where N is the .debug_info table index). Thus in this and pretty much every other case, you should be able to omit the new value. ================ 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 + ---------------- Perhaps worth including the index and table count values in this error message to make it easier for people to identify the problem. ================ Comment at: llvm/tools/obj2yaml/dwarf2yaml.cpp:19 #include <algorithm> +#include <utility> ---------------- I'm slightly surprised you needed to add this. Did this not compile without it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83116/new/ https://reviews.llvm.org/D83116 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
