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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk abandoned this revision.
kwk added a comment.

Abandoning for D69041 


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

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

[George Rimar via Phabricator via lldb-commits]

grimar added a comment.

I've posted a change for yaml2obj/obj2yaml here: D69041 
.


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you guys for jumping onto this. This will be very useful in lldb.


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

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

[George Rimar via Phabricator via lldb-commits]

grimar added a comment.

In D68943#1710525 , @MaskRay wrote:

> In D68943#1709998 , @grimar wrote:
>
> > Ok, I was able to debug it finally.
> >
> > I think we should add a .symtab in a few more cases implicitly.
> >  For example, when we have a SHT_RELA/SHT_REL section that has an empty 
> > Link,
> >  i.e. it refers to .symtab by default and we should provide it.
> >  There are other cases, like when Link just explicitly refers to .symtab.
> >
> > Here is a patch based on this one.
> >  It is unpolished, but shows the idea. With it all yaml2obj tests pass.
> >
> > F10276544: patch.diff 
>
>
> Nice! Can you post a differential for the yaml2obj change? I believe the 
> differential can focus on lldb and remove yaml2obj changes (kwk will still 
> get credited for the test cleanups).


Yes. It needs polishing + fixes for obj2yaml (I do not think it compiled for me 
at all). Also probably makes sence to add tests for obj2yaml too, I need to 
take a look what it does
when the object doesn't have symbol table with such change. I'll try to suggest 
a patch today.


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

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

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D68943#1709998 , @grimar wrote:

> Ok, I was able to debug it finally.
>
> I think we should add a .symtab in a few more cases implicitly.
>  For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
>  i.e. it refers to .symtab by default and we should provide it.
>  There are other cases, like when Link just explicitly refers to .symtab.
>
> Here is a patch based on this one.
>  It is unpolished, but shows the idea. With it all yaml2obj tests pass.
>
> F10276544: patch.diff 


Nice! Can you post a differential for the yaml2obj change? I believe the 
differential can focus on lldb and remove yaml2obj changes (kwk will still get 
credited for the test cleanups).


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

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

[George Rimar via Phabricator via lldb-commits]

grimar added a comment.

Ok, I was able to debug it finally.

I think we should add a .symtab in a few more cases implicitly.
For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
i.e. it refers to .symtab by default and we should provide it.
There are other cases, like when Link just explicitly refers to .symtab.

Here is a patch based on this one.
It is unpolished, but shows the idea. With it all yaml2obj tests pass.

F10276544: patch.diff 


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

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

[George Rimar via Phabricator via lldb-commits]

grimar added a comment.

In D68943#1709330 , @kwk wrote:

> I noticed that the order of sections does matter to some tests. Before the 
> `.symtab` section was always the first one and now it is the last of the 
> implicit sections being created. Some `Link: X`  checks break because `X` 
> points to a different section. In order to compensate for this I will make 
> `.symtab` the first of the implicit sections again.


Please upload the version with this change and I'll try to debug to take a look 
what happens.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

I noticed that the order of sections does matter to some tests. Before the 
`.symtab` section was always the first one and now it is the last of the 
implicit sections being created. Some `Link: X`  checks break because `X` 
points to a different section. In order to compensate for this I will make 
`.symtab` the first of the implicit sections again. In my local testing this 
already helps a bit. In cases where we refer to a section by number and expect 
`.symtab` to be the section being referenced, I have two options: 1st) create a 
symbol to force `.symtab` to be implicitly created or 2nd) change the name of 
the expected section. Just tell me what you want.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

@grimar I've tackled the most obvious of your comments. I will now see what 
fails in `llvm/test/tools/yaml2obj`.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 224968.
kwk marked 6 inline comments as done.
kwk added a comment.

- Remove unnecessary section
- Have consitent ordering of run, check and YAML lines
- use ## for comments in YAML
- Remove not needed Entry and Sections
- Only have Name for a symbol
- Moved test files from llvm/test/ObjectYAML/ELF/ to llvm/test/tools/yaml2obj/
- Fixup for test files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68943

Files:
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-corrupt-xz.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-find-symbols.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-no-lzma.yaml
  llvm/include/llvm/ObjectYAML/ELFYAML.h
  llvm/lib/ObjectYAML/ELFEmitter.cpp
  llvm/test/tools/yaml2obj/symtab-generated.yaml
  llvm/test/tools/yaml2obj/symtab-not-generated.yaml
  llvm/tools/obj2yaml/elf2yaml.cpp

Index: llvm/tools/obj2yaml/elf2yaml.cpp
===
--- llvm/tools/obj2yaml/elf2yaml.cpp
+++ llvm/tools/obj2yaml/elf2yaml.cpp
@@ -200,8 +200,8 @@
   return TableOrErr.takeError();
 ShndxTable = *TableOrErr;
   }
-  if (SymTab)
-if (Error E = dumpSymbols(SymTab, Y->Symbols))
+  if (SymTab && Y->Symbols)
+if (Error E = dumpSymbols(SymTab, *Y->Symbols))
   return std::move(E);
   if (DynSymTab)
 if (Error E = dumpSymbols(DynSymTab, Y->DynamicSymbols))
Index: llvm/test/tools/yaml2obj/symtab-not-generated.yaml
===
--- /dev/null
+++ llvm/test/tools/yaml2obj/symtab-not-generated.yaml
@@ -0,0 +1,15 @@
+## In this test we don't explicitly define a "Symbols" entry in the YAML and
+## expect no .symtab section to be generated.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --sections %t | FileCheck %s
+
+# CHECK-NOT: Name: .symtab
+# CHECK-NOT: Type: SHT_SYMTAB
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_REL
+  Machine: EM_386
Index: llvm/test/tools/yaml2obj/symtab-generated.yaml
===
--- /dev/null
+++ llvm/test/tools/yaml2obj/symtab-generated.yaml
@@ -0,0 +1,33 @@
+## 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 don't have .symtab listed in the "Sections" entry.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --sections %t | FileCheck -v --color --dump-input=always %s
+
+# CHECK:  Sections [
+# CHECK:Section {
+# CHECK:  Name: .symtab (19)
+# CHECK-NEXT: Type: SHT_SYMTAB (0x2)
+# CHECK-NEXT: Flags [ (0x0)
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address: 0x0
+# CHECK-NEXT: Offset: 0x68
+# CHECK-NEXT: Size: 48
+# CHECK-NEXT: Link: 1
+# CHECK-NEXT: Info: 2
+# CHECK-NEXT: AddressAlignment: 8
+# CHECK-NEXT: EntrySize: 24
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+# CHECK-EMPTY:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
+Symbols:
+- Name:main
+...
Index: llvm/lib/ObjectYAML/ELFEmitter.cpp
===
--- llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -210,8 +210,10 @@
 Doc.Sections.begin(),
 std::make_unique(
 ELFYAML::Section::SectionKind::RawContent, /*IsImplicit=*/true));
-
-  std::vector ImplicitSections = {".symtab", ".strtab", ".shstrtab"};
+  
+  std::vector ImplicitSections = {".strtab", ".shstrtab"};
+  if (Doc.Symbols) 
+ImplicitSections.push_back(".symtab");
   if (!Doc.DynamicSymbols.empty())
 ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
 
@@ -508,7 +510,7 @@
  ELFYAML::Section *YAMLSec) {
 
   bool IsStatic = STType == SymtabType::Static;
-  const auto  = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+  const auto  = (IsStatic && Doc.Symbols) ? *Doc.Symbols : Doc.DynamicSymbols;
 
   ELFYAML::RawContentSection *RawSec =
   dyn_cast_or_null(YAMLSec);
@@ -1044,14 +1046,16 @@
 }
   };
 
-  Build(Doc.Symbols, SymN2I);
+  if (Doc.Symbols)
+Build(*Doc.Symbols, SymN2I);
   Build(Doc.DynamicSymbols, DynSymN2I);
 }
 
 template  void ELFState::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
-  for (const ELFYAML::Symbol  : Doc.Symbols)
-DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
+  if (Doc.Symbols)
+for (const ELFYAML::Symbol  : *Doc.Symbols)
+  DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
   DotStrtab.finalize();
 
   // Add the dynamic symbol names to .dynstr section.
Index: llvm/include/llvm/ObjectYAML/ELFYAML.h
===
--- 

[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  = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+  const auto  = (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:   0x004004C0
+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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

In D68943#1708068 , @MaskRay wrote:

> This is not correct. A lot of yaml2obj tests fail with:
>
> > yaml2obj: error: unknown section referenced: '.symtab' by YAML section 
> > '.rela.text'


@MaskRay I'm so sorry. You're absolutely right. I was working only in LLDB for 
the last months and used `check-lldb` only. That's probably because I missed 
those errors.

> The problem is that .symtab can be the sh_link field of relocation sections, 
> .llvm_addrsig, and some sections that may be added in the future.
> 
> Making .symtab conditionally define is difficult. I'll take a closer look 
> tomorrow.

Thank you. Meanwhile I will run more tests to see what exactly fails.


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

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

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

This is not correct. A lot of yaml2obj tests fail with:

> yaml2obj: error: unknown section referenced: '.symtab' by YAML section 
> '.rela.text'

The problem is that .symtab can be the sh_link field of relocation sections, 
.llvm_addrsig, and some sections that may be added in the future.

Making .symtab conditionally define is difficult. I'll take a closer look 
tomorrow.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 224852.
kwk added a comment.

- moved test files over to llvm subtree


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68943

Files:
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-corrupt-xz.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-find-symbols.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-no-lzma.yaml
  llvm/include/llvm/ObjectYAML/ELFYAML.h
  llvm/lib/ObjectYAML/ELFEmitter.cpp
  llvm/test/ObjectYAML/ELF/no-symtab-generated.yaml
  llvm/test/ObjectYAML/ELF/symtab-generated.yaml
  llvm/tools/obj2yaml/elf2yaml.cpp

Index: llvm/tools/obj2yaml/elf2yaml.cpp
===
--- llvm/tools/obj2yaml/elf2yaml.cpp
+++ llvm/tools/obj2yaml/elf2yaml.cpp
@@ -200,8 +200,8 @@
   return TableOrErr.takeError();
 ShndxTable = *TableOrErr;
   }
-  if (SymTab)
-if (Error E = dumpSymbols(SymTab, Y->Symbols))
+  if (SymTab && Y->Symbols)
+if (Error E = dumpSymbols(SymTab, *Y->Symbols))
   return std::move(E);
   if (DynSymTab)
 if (Error E = dumpSymbols(DynSymTab, Y->DynamicSymbols))
Index: llvm/test/ObjectYAML/ELF/symtab-generated.yaml
===
--- /dev/null
+++ llvm/test/ObjectYAML/ELF/symtab-generated.yaml
@@ -0,0 +1,45 @@
+# 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 don't have .symtab listed in the "Sections" entry.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --sections %t | FileCheck %s
+
+# CHECK:  Sections [
+# CHECK:Section {
+# CHECK:  Name: .symtab (25)
+# CHECK-NEXT: Type: SHT_SYMTAB (0x2)
+# CHECK-NEXT: Flags [ (0x0)
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address: 0x0
+# CHECK-NEXT: Offset: 0x70
+# CHECK-NEXT: Size: 48
+# CHECK-NEXT: Link: 2
+# CHECK-NEXT: Info: 2
+# CHECK-NEXT: AddressAlignment: 8
+# CHECK-NEXT: EntrySize: 24
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+# CHECK-EMPTY:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
+  Entry:   0x004004C0
+Sections:
+- Name:.text
+  Type:SHT_PROGBITS
+  Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+  Address: 0x004003D0
+  AddressAlign:0x0010
+  Content: DEADBEEFBAADF00D
+Symbols:
+- Name:main
+  Type:STT_FUNC
+  Section: .text
+  Value:   0x004003D0
+  Size:0x0008
+...
Index: llvm/test/ObjectYAML/ELF/no-symtab-generated.yaml
===
--- /dev/null
+++ llvm/test/ObjectYAML/ELF/no-symtab-generated.yaml
@@ -0,0 +1,19 @@
+# In this test we don't explicitly define a "Symbols" entry in the YAML and
+# expect no .symtab section to be generated.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --sections %t | FileCheck %s
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_REL
+  Machine: EM_386
+Sections:
+  - Name:.debug_line
+Type:SHT_PROGBITS
+Flags:   [ SHF_COMPRESSED ]
+
+# CHECK-NOT: Name: .symtab
+# CHECK-NOT: Type: SHT_SYMTAB
Index: llvm/lib/ObjectYAML/ELFEmitter.cpp
===
--- llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -210,8 +210,10 @@
 Doc.Sections.begin(),
 std::make_unique(
 ELFYAML::Section::SectionKind::RawContent, /*IsImplicit=*/true));
-
-  std::vector ImplicitSections = {".symtab", ".strtab", ".shstrtab"};
+  
+  std::vector ImplicitSections = {".strtab", ".shstrtab"};
+  if (Doc.Symbols) 
+ImplicitSections.push_back(".symtab");
   if (!Doc.DynamicSymbols.empty())
 ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
 
@@ -508,7 +510,7 @@
  ELFYAML::Section *YAMLSec) {
 
   bool IsStatic = STType == SymtabType::Static;
-  const auto  = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+  const auto  = (IsStatic && Doc.Symbols) ? *Doc.Symbols : Doc.DynamicSymbols;
 
   ELFYAML::RawContentSection *RawSec =
   dyn_cast_or_null(YAMLSec);
@@ -1044,14 +1046,16 @@
 }
   };
 
-  Build(Doc.Symbols, SymN2I);
+  if (Doc.Symbols)
+Build(*Doc.Symbols, SymN2I);
   Build(Doc.DynamicSymbols, DynSymN2I);
 }
 
 template  void ELFState::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
-  for (const ELFYAML::Symbol  : Doc.Symbols)
-DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
+  if (Doc.Symbols)
+for (const ELFYAML::Symbol  : *Doc.Symbols)
+  DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
  

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk marked an inline comment as done.
kwk added a comment.

Moved `yaml2obj` tests under llvm subtree.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk planned changes to this revision.
kwk added a comment.

Move tests over to `llvm/llvm/test/ObjectYAML/ELF/`.


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

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

[Pavel Labath via Phabricator via lldb-commits]

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 Symbols;
+  Optional> Symbols;
   std::vector 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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk marked an inline comment as done.
kwk added inline comments.



Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:379-380
   // being a single SHT_SYMTAB section are upheld.
-  std::vector Symbols;
+  Optional> Symbols;
   std::vector DynamicSymbols;
 };

labath wrote:
> 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.)
@labath I haven't looked but will investigate how hard it is to declare 
`Optional> DynamicSymbols;`. That's what I understood at 
least.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 224839.
kwk added a comment.

- restore formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68943

Files:
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-corrupt-xz.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-find-symbols.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-no-lzma.yaml
  lldb/test/Shell/ObjectFile/ELF/no-symtab-generated.yaml
  lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
  llvm/include/llvm/ObjectYAML/ELFYAML.h
  llvm/lib/ObjectYAML/ELFEmitter.cpp
  llvm/tools/obj2yaml/elf2yaml.cpp

Index: llvm/tools/obj2yaml/elf2yaml.cpp
===
--- llvm/tools/obj2yaml/elf2yaml.cpp
+++ llvm/tools/obj2yaml/elf2yaml.cpp
@@ -200,8 +200,8 @@
   return TableOrErr.takeError();
 ShndxTable = *TableOrErr;
   }
-  if (SymTab)
-if (Error E = dumpSymbols(SymTab, Y->Symbols))
+  if (SymTab && Y->Symbols)
+if (Error E = dumpSymbols(SymTab, *Y->Symbols))
   return std::move(E);
   if (DynSymTab)
 if (Error E = dumpSymbols(DynSymTab, Y->DynamicSymbols))
Index: llvm/lib/ObjectYAML/ELFEmitter.cpp
===
--- llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -210,8 +210,10 @@
 Doc.Sections.begin(),
 std::make_unique(
 ELFYAML::Section::SectionKind::RawContent, /*IsImplicit=*/true));
-
-  std::vector ImplicitSections = {".symtab", ".strtab", ".shstrtab"};
+  
+  std::vector ImplicitSections = {".strtab", ".shstrtab"};
+  if (Doc.Symbols) 
+ImplicitSections.push_back(".symtab");
   if (!Doc.DynamicSymbols.empty())
 ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
 
@@ -508,7 +510,7 @@
  ELFYAML::Section *YAMLSec) {
 
   bool IsStatic = STType == SymtabType::Static;
-  const auto  = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+  const auto  = (IsStatic && Doc.Symbols) ? *Doc.Symbols : Doc.DynamicSymbols;
 
   ELFYAML::RawContentSection *RawSec =
   dyn_cast_or_null(YAMLSec);
@@ -1044,14 +1046,16 @@
 }
   };
 
-  Build(Doc.Symbols, SymN2I);
+  if (Doc.Symbols)
+Build(*Doc.Symbols, SymN2I);
   Build(Doc.DynamicSymbols, DynSymN2I);
 }
 
 template  void ELFState::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
-  for (const ELFYAML::Symbol  : Doc.Symbols)
-DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
+  if (Doc.Symbols)
+for (const ELFYAML::Symbol  : *Doc.Symbols)
+  DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
   DotStrtab.finalize();
 
   // Add the dynamic symbol names to .dynstr section.
Index: llvm/include/llvm/ObjectYAML/ELFYAML.h
===
--- llvm/include/llvm/ObjectYAML/ELFYAML.h
+++ llvm/include/llvm/ObjectYAML/ELFYAML.h
@@ -376,7 +376,7 @@
   // cleaner and nicer if we read them from the YAML as a separate
   // top-level key, which automatically ensures that invariants like there
   // being a single SHT_SYMTAB section are upheld.
-  std::vector Symbols;
+  Optional> Symbols;
   std::vector DynamicSymbols;
 };
 
Index: lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
@@ -0,0 +1,37 @@
+# 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 don't have .symtab listed in the "Sections" entry.
+
+# RUN: yaml2obj %s > %t.obj
+# RUN: %lldb -b -o "image dump sections" %t.obj | FileCheck %s
+
+# CHECK: SectID Type File Address Perm File Off.  File Size  Flags  Section Name
+# CHECK-NEXT: --  ---   -- -- -- 
+# CHECK-NEXT: {{.*}}.text
+# CHECK-NEXT: {{.*}}.strtab
+# CHECK-NEXT: {{.*}}.shstrtab
+# CHECK-NEXT: {{.*}}.symtab
+# CHECK-EMPTY:
+
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
+  Entry:   0x004004C0
+Sections:
+- Name:.text
+  Type:SHT_PROGBITS
+  Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+  Address: 0x004003D0
+  AddressAlign:0x0010
+  Content: DEADBEEFBAADF00D
+Symbols:
+- Name:main
+  Type:STT_FUNC
+  Section: .text
+  Value:   0x004003D0
+  Size:0x0008
+...
Index: lldb/test/Shell/ObjectFile/ELF/no-symtab-generated.yaml
===
--- /dev/null
+++ 

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk created this revision.
kwk added a reviewer: labath.
Herald added subscribers: llvm-commits, lldb-commits, MaskRay, hiraditya, 
arichardson, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: alexshap.
Herald added projects: LLDB, LLVM.
kwk updated this revision to Diff 224838.
kwk added a comment.

- Silence FileCheck in test


Before `yaml2obj ` would always add an implicit `.symtab` section
even if there were no symbols defined in the YAML ``. Now, only
when there's a `Symbols` entry, we will generate a `.symtab` section.

Old minidebuginfo tests that manually removed the `.symtab` section
have been adjusted because it is no longer needed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68943

Files:
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-corrupt-xz.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-find-symbols.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-no-lzma.yaml
  lldb/test/Shell/ObjectFile/ELF/no-symtab-generated.yaml
  lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
  llvm/include/llvm/ObjectYAML/ELFYAML.h
  llvm/lib/ObjectYAML/ELFEmitter.cpp
  llvm/tools/obj2yaml/elf2yaml.cpp

Index: llvm/tools/obj2yaml/elf2yaml.cpp
===
--- llvm/tools/obj2yaml/elf2yaml.cpp
+++ llvm/tools/obj2yaml/elf2yaml.cpp
@@ -200,8 +200,8 @@
   return TableOrErr.takeError();
 ShndxTable = *TableOrErr;
   }
-  if (SymTab)
-if (Error E = dumpSymbols(SymTab, Y->Symbols))
+  if (SymTab && Y->Symbols)
+if (Error E = dumpSymbols(SymTab, *Y->Symbols))
   return std::move(E);
   if (DynSymTab)
 if (Error E = dumpSymbols(DynSymTab, Y->DynamicSymbols))
Index: llvm/lib/ObjectYAML/ELFEmitter.cpp
===
--- llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -210,8 +210,10 @@
 Doc.Sections.begin(),
 std::make_unique(
 ELFYAML::Section::SectionKind::RawContent, /*IsImplicit=*/true));
-
-  std::vector ImplicitSections = {".symtab", ".strtab", ".shstrtab"};
+  
+  std::vector ImplicitSections = {".strtab", ".shstrtab"};
+  if (Doc.Symbols) 
+ImplicitSections.push_back(".symtab");
   if (!Doc.DynamicSymbols.empty())
 ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
 
@@ -508,7 +510,7 @@
  ELFYAML::Section *YAMLSec) {
 
   bool IsStatic = STType == SymtabType::Static;
-  const auto  = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+  const auto  = (IsStatic && Doc.Symbols) ? *Doc.Symbols : Doc.DynamicSymbols;
 
   ELFYAML::RawContentSection *RawSec =
   dyn_cast_or_null(YAMLSec);
@@ -556,8 +558,9 @@
 
   // If the symbol table section is explicitly described in the YAML
   // then we should set the fields requested.
-  SHeader.sh_info = (RawSec && RawSec->Info) ? (unsigned)(*RawSec->Info)
- : findFirstNonGlobal(Symbols) + 1;
+  SHeader.sh_info = (RawSec && RawSec->Info)
+? (unsigned)(*RawSec->Info)
+: findFirstNonGlobal(Symbols) + 1;
   SHeader.sh_entsize = (YAMLSec && YAMLSec->EntSize)
? (uint64_t)(*YAMLSec->EntSize)
: sizeof(Elf_Sym);
@@ -1044,14 +1047,16 @@
 }
   };
 
-  Build(Doc.Symbols, SymN2I);
+  if (Doc.Symbols)
+Build(*Doc.Symbols, SymN2I);
   Build(Doc.DynamicSymbols, DynSymN2I);
 }
 
 template  void ELFState::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
-  for (const ELFYAML::Symbol  : Doc.Symbols)
-DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
+  if (Doc.Symbols)
+for (const ELFYAML::Symbol  : *Doc.Symbols)
+  DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
   DotStrtab.finalize();
 
   // Add the dynamic symbol names to .dynstr section.
Index: llvm/include/llvm/ObjectYAML/ELFYAML.h
===
--- llvm/include/llvm/ObjectYAML/ELFYAML.h
+++ llvm/include/llvm/ObjectYAML/ELFYAML.h
@@ -376,7 +376,7 @@
   // cleaner and nicer if we read them from the YAML as a separate
   // top-level key, which automatically ensures that invariants like there
   // being a single SHT_SYMTAB section are upheld.
-  std::vector Symbols;
+  Optional> Symbols;
   std::vector DynamicSymbols;
 };
 
Index: lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
@@ -0,0 +1,37 @@
+# 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 don't have .symtab listed in the "Sections" entry.
+
+# RUN: yaml2obj %s > %t.obj
+# RUN: %lldb -b -o "image dump sections" %t.obj | FileCheck %s
+
+# CHECK: SectID Type File 

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 224838.
kwk added a comment.

- Silence FileCheck in test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68943

Files:
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-corrupt-xz.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-find-symbols.yaml
  lldb/test/Shell/ObjectFile/ELF/minidebuginfo-no-lzma.yaml
  lldb/test/Shell/ObjectFile/ELF/no-symtab-generated.yaml
  lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
  llvm/include/llvm/ObjectYAML/ELFYAML.h
  llvm/lib/ObjectYAML/ELFEmitter.cpp
  llvm/tools/obj2yaml/elf2yaml.cpp

Index: llvm/tools/obj2yaml/elf2yaml.cpp
===
--- llvm/tools/obj2yaml/elf2yaml.cpp
+++ llvm/tools/obj2yaml/elf2yaml.cpp
@@ -200,8 +200,8 @@
   return TableOrErr.takeError();
 ShndxTable = *TableOrErr;
   }
-  if (SymTab)
-if (Error E = dumpSymbols(SymTab, Y->Symbols))
+  if (SymTab && Y->Symbols)
+if (Error E = dumpSymbols(SymTab, *Y->Symbols))
   return std::move(E);
   if (DynSymTab)
 if (Error E = dumpSymbols(DynSymTab, Y->DynamicSymbols))
Index: llvm/lib/ObjectYAML/ELFEmitter.cpp
===
--- llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -210,8 +210,10 @@
 Doc.Sections.begin(),
 std::make_unique(
 ELFYAML::Section::SectionKind::RawContent, /*IsImplicit=*/true));
-
-  std::vector ImplicitSections = {".symtab", ".strtab", ".shstrtab"};
+  
+  std::vector ImplicitSections = {".strtab", ".shstrtab"};
+  if (Doc.Symbols) 
+ImplicitSections.push_back(".symtab");
   if (!Doc.DynamicSymbols.empty())
 ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
 
@@ -508,7 +510,7 @@
  ELFYAML::Section *YAMLSec) {
 
   bool IsStatic = STType == SymtabType::Static;
-  const auto  = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+  const auto  = (IsStatic && Doc.Symbols) ? *Doc.Symbols : Doc.DynamicSymbols;
 
   ELFYAML::RawContentSection *RawSec =
   dyn_cast_or_null(YAMLSec);
@@ -556,8 +558,9 @@
 
   // If the symbol table section is explicitly described in the YAML
   // then we should set the fields requested.
-  SHeader.sh_info = (RawSec && RawSec->Info) ? (unsigned)(*RawSec->Info)
- : findFirstNonGlobal(Symbols) + 1;
+  SHeader.sh_info = (RawSec && RawSec->Info)
+? (unsigned)(*RawSec->Info)
+: findFirstNonGlobal(Symbols) + 1;
   SHeader.sh_entsize = (YAMLSec && YAMLSec->EntSize)
? (uint64_t)(*YAMLSec->EntSize)
: sizeof(Elf_Sym);
@@ -1044,14 +1047,16 @@
 }
   };
 
-  Build(Doc.Symbols, SymN2I);
+  if (Doc.Symbols)
+Build(*Doc.Symbols, SymN2I);
   Build(Doc.DynamicSymbols, DynSymN2I);
 }
 
 template  void ELFState::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
-  for (const ELFYAML::Symbol  : Doc.Symbols)
-DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
+  if (Doc.Symbols)
+for (const ELFYAML::Symbol  : *Doc.Symbols)
+  DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
   DotStrtab.finalize();
 
   // Add the dynamic symbol names to .dynstr section.
Index: llvm/include/llvm/ObjectYAML/ELFYAML.h
===
--- llvm/include/llvm/ObjectYAML/ELFYAML.h
+++ llvm/include/llvm/ObjectYAML/ELFYAML.h
@@ -376,7 +376,7 @@
   // cleaner and nicer if we read them from the YAML as a separate
   // top-level key, which automatically ensures that invariants like there
   // being a single SHT_SYMTAB section are upheld.
-  std::vector Symbols;
+  Optional> Symbols;
   std::vector DynamicSymbols;
 };
 
Index: lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/symtab-generated.yaml
@@ -0,0 +1,37 @@
+# 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 don't have .symtab listed in the "Sections" entry.
+
+# RUN: yaml2obj %s > %t.obj
+# RUN: %lldb -b -o "image dump sections" %t.obj | FileCheck %s
+
+# CHECK: SectID Type File Address Perm File Off.  File Size  Flags  Section Name
+# CHECK-NEXT: --  ---   -- -- -- 
+# CHECK-NEXT: {{.*}}.text
+# CHECK-NEXT: {{.*}}.strtab
+# CHECK-NEXT: {{.*}}.shstrtab
+# CHECK-NEXT: {{.*}}.symtab
+# CHECK-EMPTY:
+
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: