[Lldb-commits] [PATCH] D68943: [llvm][yaml2obj] no more implicit ELF .symtab section

[George Rimar via Phabricator via lldb-commits]

grimar added a comment.

I have not looked what is needed to fix yaml2obj testcases yet, but below there are
few first comments.

Also, 2 LLVM side tests added should live in "llvm/test/tools/yaml2obj" I think (not in llvm/test/ObjectYAML/ELF/),
because this is actually the place where we test "yaml2obj" usually.



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:513
   bool IsStatic = STType == SymtabType::Static;
-  const auto &Symbols = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+  const auto &Symbols = (IsStatic && Doc.Symbols) ? *Doc.Symbols : Doc.DynamicSymbols;
 
----------------
It doesn't look correct. We should never use dynamic symbols instead of static symbols I believe.


================
Comment at: llvm/test/ObjectYAML/ELF/no-symtab-generated.yaml:14
+Sections:
+  - Name:            .debug_line
+    Type:            SHT_PROGBITS
----------------
You do not need any sections.


================
Comment at: llvm/test/ObjectYAML/ELF/no-symtab-generated.yaml:18
+
+# CHECK-NOT: Name: .symtab
+# CHECK-NOT: Type: SHT_SYMTAB
----------------
We often use the following order:
1) Run lines
2) Check lines
3) YAML

I.e. the order you have used in symtab-generated.yaml. I'd suggest to be consistent and stick to it.

Also, probably would be better to have a single test case file, i.e. to merge them. You can see other our tests which do that.


================
Comment at: llvm/test/ObjectYAML/ELF/symtab-generated.yaml:1
+# In this test we define a "Symbols" entry in the YAML and expect a .symtab
+# section to be generated. Notice that this is automatically added even though
----------------
We usually use ## for comments in yaml2obj tests.


================
Comment at: llvm/test/ObjectYAML/ELF/symtab-generated.yaml:31
+  Machine:         EM_X86_64
+  Entry:           0x00000000004004C0
+Sections:
----------------
You do not need Entry.


================
Comment at: llvm/test/ObjectYAML/ELF/symtab-generated.yaml:33
+Sections:
+- Name:            .text
+  Type:            SHT_PROGBITS
----------------
You do not need any sections it seems.


================
Comment at: llvm/test/ObjectYAML/ELF/symtab-generated.yaml:41
+- Name:            main
+  Type:            STT_FUNC
+  Section:         .text
----------------
Having only a "Name" should be enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68943