labath added reviewers: grimar, MaskRay, jhenderson. labath added a comment.
+llvm yaml2obj folks. Thank you for creating this diff. Please see comments inline. ================ Comment at: lldb/test/Shell/ObjectFile/ELF/no-symtab-generated.yaml:1 +# In this test we don't explicitly define a "Symbols" entry in the YAML and +# expect no .symtab section to be generated. ---------------- This test (and the next one) should live in the llvm subtree, as they're really testing yaml2obj, not lldb (instead of lldb, you can use something like llvm-objdump/readobj to inspect the generated files). ================ Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:379-380 // being a single SHT_SYMTAB section are upheld. - std::vector<Symbol> Symbols; + Optional<std::vector<Symbol>> Symbols; std::vector<Symbol> DynamicSymbols; }; ---------------- This creates an inconsistency between how .symtab and .dynsym sections are handled, which is hard to justify. I actually prefer the Optional<> version, because that enables one to say "the symtab section is there, but it is _empty_", something which is impossible with the way .dynsym emission works (the section gets surpressed if it is empty), but the question is, shouldn't then the .dynsym emission work the same way ? Have you looked at by any chance how hard it would be to change .dynsym to follow the same pattern? (In any case, I think this is up to yaml2obj maintainers to decide how to handle this. I'll add some to this review.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68943/new/ https://reviews.llvm.org/D68943 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
