[yaml2obj/obj2yaml] - Allow having the symbols and sections with duplicated names.

[grimar]

Summary

Imagine we create an object from the following asm:

.section .foo1,"ax",%progbits
nop
.section .foo,"ax",%progbits,unique,1
nop
.section .foo,"ax",%progbits,unique,2
nop
.section .foo2,"ax",%progbits
nop

I.e. object has sections .foo1, .foo, .foo and .foo2.

obj2yaml has a broken logic and will produce YAML that has
the following sections: .foo1, .foo, .foo2, .foo23.

And there is no way to write a YAML that would allow yaml2obj
to produce the object with duplicated section names.

Now, lets take a look at a different case:

file1.s:

.section .text.foo.1
.local localfoo
localfoo:
nop

.section .text.1,"ax",%progbits
.quad localfoo

file2.s:

.section .text.foo.2
.local localfoo
localfoo:
nop

.section .text.2,"ax",%progbits
.quad localfoo

If we build the objects and link them with -r: ld.bfd -r file1.o file2.o -o out
then the resulting object will contain 2 symbols with the name localfoo.

obj2yaml can produce the YAML, but yaml2obj will fail to use it:
error: Repeated symbol name: 'localfoo'.

The solution I suggest in this patch is to use a special suffix
to encode the duplicated names of symbols and sections.
Test cases show the idea. The advantage is that the new style is simple and
code as almost not changed, though both describe issues are fixed.

BTW after I implemented it I had to fix the existent test and found that
my solution seems to be close (or maybe even the same) to one of the ideas mentioned by
Rafael in the rL312585 commit.

[grimar]

I am going to recheck something here after last minute changes done.

[grimar]

I'll update the implementation and add a test case soon.

[grimar]

• Changed implementation, added test cases.

[MaskRay]

Nice! Can you add another test that checks if a symbol can be defined relative to a section with duplicates? It can probably be placed in duplicate-section-names.test

[jhenderson]

Hmmm... I'm not sure we need any special behaviour at all in the name field of the syntax, and the suggested syntax is not obvious to me. Surely if yaml2obj sees two symbols with the same name, it should just generate two such symbols?

I guess the issue really is how to reference them, but I don't think that should be done through the name field of the symbol personally. I'm still thinking about this, but you could have a separate ID field, which is required to be unique, and allow references be via ID? Example:

Symbols:
  - Name: foo
    ID: foo1 # error if symbol name of foo1 exists
  - Name: foo
    ID: foo2
Sections:
  - Name: .rela
    Type: SHT_RELA
    Relocations:
      - Offset: 0
        Type: R_X86_64_NONE
        Symbol: foo1 # use ID
      - Offset: 1
        Type: R_X86_64_NONE
        Symbol: foo # error - ambiguous

# Possible alternative:
      - Offset: 0
        Type: R_X86_64_NONE
        Symbol: foo # use name, but if multiple found, fallback to ID; error if no ID specified during fallback.
        ID: foo1

The same syntax would work for sections.

I'd also like to see more testing of referencing these unique sections and symbols. At the moment, it seems limited to what there is in the obj2yaml test case. I think you should add some dedicated yaml2obj tests to show that the references work.

test/tools/obj2yaml/duplicate-symbol-names.test

1–2 ↗

(On Diff #205874)

"produce YAML from an object"

34 ↗

(On Diff #205874)

Remove "the"

75 ↗

(On Diff #205874)

to a name

test/tools/yaml2obj/duplicate-section-names.test

1–2 ↗

(On Diff #205874)

from YAML
containing sections

test/tools/yaml2obj/duplicate-symbol-names.test

1–2 ↗

(On Diff #205874)

from YAML

20 ↗

(On Diff #205874)

in case -> when

36 ↗

(On Diff #205874)

in case -> when

tools/obj2yaml/elf2yaml.cpp

157 ↗

(On Diff #205874)

that -> this

another -> other

[grimar]

In D63596#1553209, @jhenderson wrote:

Hmmm... I'm not sure we need any special behaviour at all in the name field of the syntax, and the suggested syntax is not obvious to me. Surely if yaml2obj sees two symbols with the same name, it should just generate two such symbols?

I guess the issue really is how to reference them, but I don't think that should be done through the name field of the symbol personally. I'm still thinking about this, but you could have a separate ID field, which is required to be unique, and allow references be via ID? Example:

Symbols:
  - Name: foo
    ID: foo1 # error if symbol name of foo1 exists
  - Name: foo
    ID: foo2
Sections:
  - Name: .rela
    Type: SHT_RELA
    Relocations:
      - Offset: 0
        Type: R_X86_64_NONE
        Symbol: foo1 # use ID
      - Offset: 1
        Type: R_X86_64_NONE
        Symbol: foo # error - ambiguous

# Possible alternative:
      - Offset: 0
        Type: R_X86_64_NONE
        Symbol: foo # use name, but if multiple found, fallback to ID; error if no ID specified during fallback.
        ID: foo1

The same syntax would work for sections.

I experimented with the code a lot while worked on this patch and having an additional index was a solution
I also considered, but dropped this idea.

At this moment we use NameToIdxMap class to lookup symbol or section index by name in many places.
What you suggest needs a major rework of the code. This change will introduce additional complexity for all cases,
even when we have no duplications at all (99% of all cases I guess?).

The logic to "use name, but if multiple found, fallback to ID" seems overcomplicated to me,
it will be hard to read and maintain such YAML. Maybe we can reference symbols only by ID in all cases,
but it is also inconvenient way generally and is harmful to readability.

I do not think it is a very often case to have duplicated symbol/sections name, so I selected the less intrusive way.

[jhenderson]

I do not think it is a very often case to have duplicated symbol/sections name, so I selected the less intrusive way.

Okay, I don't mind. To me, most important is that you can specify multiple sections/symbols with identical names, since I've wanted to do that on more than one occasion. It might be nice if you could accept the following and auto-create the suffixes internally, but I don't feel strongly about it:

Sections:
  - Name: foo
    Type: SHT_PROGBITS
  - Name: foo
    Type: SHT_PROGBITS

[MaskRay]

tools/obj2yaml/elf2yaml.cpp

35 ↗

(On Diff #205874)

Suffix is shared: we may get .a .a [1] .b .b [2] .b [3]

Can DenseSet<StringRef> UsedSectionNames; be changed to DenseMap<StringRef, int> to have a per-section suffix number?

98 ↗

(On Diff #205874)

Twine(++Suffix) eliminates the cost to construct a string.

[grimar]

• Addressed review comments, added test cases.

In D63596#1553546, @jhenderson wrote:

I do not think it is a very often case to have duplicated symbol/sections name, so I selected the less intrusive way.

Okay, I don't mind. To me, most important is that you can specify multiple sections/symbols with identical names, since I've wanted to do that on more than one occasion. It might be nice if you could accept the following and auto-create the suffixes internally, but I don't feel strongly about it:

Sections:
  - Name: foo
    Type: SHT_PROGBITS
  - Name: foo
    Type: SHT_PROGBITS

It is not hard to support this trivial case, but the problem is how to support more complex cases.
For example, if we also have a relocation section declaration:

  - Name: .rela.text
    Type: SHT_RELA
    Info: .text
    Link: .symtab
    Relocations:
      - Offset: 0x0
        Type:   R_X86_64_NONE
        Symbol: foo
      - Offset: 0x1
        Type:   R_X86_64_NONE
        Symbol: foo
....
Sections:
  - Name: foo
    Type: SHT_PROGBITS
  - Name: foo
    Type: SHT_PROGBITS

Then we have ambiguity and probably want to detect it and report an error.
The same applies to other cases, e.g. for group section declarations.

If we really want to support only the trivial case mentioned,
I would do it in a separate patch with own test cases.
(I am also not sure, I would probably just improve the error reported to include the suggestion to add a suffix. Since such a situation can only happen with handwritten YAMLs after this patch)

tools/obj2yaml/elf2yaml.cpp

35 ↗

(On Diff #205874)

Ok, done. I updated CASE1 in obj2yaml\duplicate-symbol-names.test to test the new logic.

[jhenderson]

test/Object/X86/obj2yaml-dup-section-name.s

1 ↗

(On Diff #206186)

I think you still need test cases in both section and symbol cases that show the unique ID is unique for subsequent sections of the same name (i.e. you need a .text.foo, a .text.foo [1], and a .text.foo [2] at least).

test/tools/yaml2obj/duplicate-section-names.test

66 ↗

(On Diff #206186)

relative to sections

110–111 ↗

(On Diff #206186)

This isn't a clear sentence to me. Perhaps the following:

"Check that yaml2obj can produce SHT_GROUP sections that reference sections and symbols with name suffixes."

I might even change the last part to "name ID suffixes", because it is not necessarily clear what the suffixes refer to, but then again, this is probably not needed in the context of this test.

test/tools/yaml2obj/duplicate-symbol-names.test

55–57 ↗

(On Diff #206186)

This comment can be rewritten slightly, in a similar manner to the group one:

"Check that yaml2obj can produce correct relocations that reference symbols with name suffixes."

[grimar]

test/Object/X86/obj2yaml-dup-section-name.s

1 ↗

(On Diff #206186)

I did what you describe in CASE1 of duplicate-symbol-names.test I think?
I'll add a one more symbol and one more section there.

This one can probably be removed, but I am not sure, since it belongs to test/Object and in theory
tests how we dump the object produced with llvm-mc. So I leaved it along.

[grimar]

• Addressed review comments.

[jhenderson]

test/Object/X86/obj2yaml-dup-section-name.s

1 ↗

(On Diff #206186)

Okay, I see what caused some of my confusion. duplicate-symbol-names.test tests both symbol and section names. It probably needs renaming to something that doesn't suggest it's just about symbol names...!

[grimar]

• Renamed the test case, updated its 'CASE1' comment to mention not only symbols, but also sections.

test/Object/X86/obj2yaml-dup-section-name.s

1 ↗

(On Diff #206186)

Renamed to duplicate-symbol-and-section-names.test :]

[jhenderson]

test/tools/obj2yaml/duplicate-symbol-and-section-names.test

4 ↗

(On Diff #206197)

reflects -> reflect

I'm not sure I understand what "index of duplication" means. Probably sufficient to say that the suffixes are unique (it really doesn't matter how they are unique e.g. with index).

[grimar]

test/tools/obj2yaml/duplicate-symbol-and-section-names.test

4 ↗

(On Diff #206197)

I'm not sure I understand what "index of duplication" means.

Order number of the duplication? I.e. [1] below means this is the first duplication for localfoo,
[2] means the second and so on.

# CASE1-NEXT:   - Name: localfoo
# CASE1-NEXT:   - Name: 'localfoo [1]'
# CASE1-NEXT:   - Name: 'localfoo [2]'

Probably sufficient to say that the suffixes are unique

They are not. They were unique in the first versions of this patch,
but now we can have both localfoo [1] and localbar [1] for example.

(it really doesn't matter how they are unique e.g. with index).

It does not matter for yaml2obj. but here we testing obj2yaml output,
which has a special logic to produce
localfoo, 'localfoo [1]', localbar, 'localbar [1]'
instead of localfoo, 'localfoo [1]', localbar, 'localbar [2]'

That is what I tried to explain in the comment.

What about the following?
"and that the suffixes produced for each section and symbol shows the order number
of the duplication"

[jhenderson]

test/tools/obj2yaml/duplicate-symbol-and-section-names.test

4 ↗

(On Diff #206197)

To be clear, I did eventually understand it, but the wording isn't how I'd word things.

I don't think it really matters what obj2yaml produces, as long as each different section with the same name has different suffixes in the output, so references to the order are irrelevant. As far as I am concerned, obj2yaml could write them as "localbar [bear]", "localbar [cat]", "localbar [dog]" etc. Ideally, I'd have this test simply check that the numbers between '[' and ']' are different to each other, for matching section/symbol names, but that's not practical.

How about "and produces same-named sections and symbols with distinguishing suffixes."?

[grimar]

• Addressed review comment.

test/tools/obj2yaml/duplicate-symbol-and-section-names.test

4 ↗

(On Diff #206197)

OK, done.

[jhenderson]

LGTM.

[MaskRay]

tools/obj2yaml/elf2yaml.cpp

269 ↗

(On Diff #206221)

superfluous ()?

tools/yaml2obj/yaml2elf.cpp

273 ↗

(On Diff #206221)

.substr

This should ease transition to std::string_view when llvm moves to C++17... string_view doesn't provide slice.

llvm/trunk/test/Object/X86/obj2yaml-dup-section-name.s

 # RUN: llvm-mc %s -o %t.o -filetype=obj -triple=x86_64-pc-linux
 # RUN: obj2yaml %t.o | FileCheck %s
 
 # CHECK: Sections:
 # CHECK: - Name:            .group{{$}}
 # CHECK:   Members:
 # CHECK:     - SectionOrType:   .text.foo{{$}}
 # CHECK:     - SectionOrType:   .rela.text.foo{{$}}
 # CHECK: - Name:            .text.foo{{$}}
 # CHECK: - Name:            .rela.text.foo{{$}}
 # CHECK:   Info:            .text.foo{{$}}
-# CHECK: - Name:            .group1{{$}}
+# CHECK: - Name:            '.group [1]'
 # CHECK:   Members:
-# CHECK:     - SectionOrType:   .text.foo2{{$}}
-# CHECK:     - SectionOrType:   .rela.text.foo3{{$}}
-# CHECK: - Name:            .text.foo2{{$}}
-# CHECK: - Name:            .rela.text.foo3{{$}}
-# CHECK:   Info:            .text.foo2{{$}}
+# CHECK:     - SectionOrType:   '.text.foo [1]'
+# CHECK:     - SectionOrType:   '.rela.text.foo [1]'
+# CHECK: - Name:            '.text.foo [1]'
+# CHECK: - Name:            '.rela.text.foo [1]'
+# CHECK:   Info:            '.text.foo [1]'
 # CHECK: Symbols:
 # CHECK:   Section:         .group{{$}}
-# CHECK:   Section:         .group1{{$}}
+# CHECK:   Section:         '.group [1]'
 
 
         .section        .text.foo,"axG",@progbits,sym1,comdat
         .quad undef
 
         .section        .text.foo,"axG",@progbits,sym2,comdat
         .quad undef

llvm/trunk/test/tools/obj2yaml/duplicate-symbol-and-section-names.test

+## Check that obj2yaml is able to produce YAML from
+## an object containing symbols and sections with duplicate
+## names and produces same-named sections and symbols
+## with distinguishing suffixes.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj -s -t %t1 | FileCheck %s
+
+# CHECK: Name: .foo (
+# CHECK: Name: .foo (
+# CHECK: Name: .foo (
+# CHECK: Name: .bar (
+# CHECK: Name: .bar (
+# CHECK: Name: .bar (
+
+# CHECK: Name: localfoo (
+# CHECK: Name: localfoo (
+# CHECK: Name: localfoo (
+# CHECK: Name: localbar (
+# CHECK: Name: localbar (
+# CHECK: Name: localbar (
+
+# RUN: obj2yaml %t1 | FileCheck %s --check-prefix=CASE1
+
+# CASE1:      --- !ELF
+# CASE1-NEXT: FileHeader:
+# CASE1-NEXT:   Class:   ELFCLASS64
+# CASE1-NEXT:   Data:    ELFDATA2LSB
+# CASE1-NEXT:   Type:    ET_REL
+# CASE1-NEXT:   Machine: EM_X86_64
+# CASE1-NEXT: Sections:
+# CASE1-NEXT:   - Name: .foo
+# CASE1-NEXT:     Type: SHT_PROGBITS
+# CASE1-NEXT:   - Name: '.foo [1]'
+# CASE1-NEXT:     Type: SHT_PROGBITS
+# CASE1-NEXT:   - Name: '.foo [2]'
+# CASE1-NEXT:     Type: SHT_PROGBITS
+# CASE1-NEXT:   - Name: .bar
+# CASE1-NEXT:     Type: SHT_PROGBITS
+# CASE1-NEXT:   - Name: '.bar [1]'
+# CASE1-NEXT:     Type: SHT_PROGBITS
+# CASE1-NEXT:   - Name: '.bar [2]'
+# CASE1-NEXT:     Type: SHT_PROGBITS
+# CASE1-NEXT: Symbols:
+# CASE1-NEXT:   - Name: localfoo
+# CASE1-NEXT:   - Name: 'localfoo [1]'
+# CASE1-NEXT:   - Name: 'localfoo [2]'
+# CASE1-NEXT:   - Name: localbar
+# CASE1-NEXT:   - Name: 'localbar [1]'
+# CASE1-NEXT:   - Name: 'localbar [2]'
+# CASE1-NEXT: ...
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+  - Name: '.foo [555]'
+    Type: SHT_PROGBITS
+  - Name: '.foo [random_tag]'
+    Type: SHT_PROGBITS
+  - Name: .bar
+    Type: SHT_PROGBITS
+  - Name: '.bar [666]'
+    Type: SHT_PROGBITS
+  - Name: '.bar [random_tag]'
+    Type: SHT_PROGBITS
+Symbols:
+  - Name: 'localfoo [111]'
+  - Name: 'localfoo [222]'
+  - Name: 'localfoo [random_tag]' 
+  - Name: 'localbar [333]'
+  - Name: 'localbar [444]'
+  - Name: 'localbar [random_tag]' 
+
+## Check we can refer to symbols with the same
+## name from relocations.
+
+# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: obj2yaml %t2 | FileCheck %s --check-prefix=CASE2
+
+# CASE2:      Relocations:
+# CASE2-NEXT:  - Offset: 0x0000000000000000
+# CASE2-NEXT:    Symbol: 'foo [1]'
+# CASE2-NEXT:    Type:   R_X86_64_PC32
+# CASE2-NEXT:  - Offset: 0x0000000000000004
+# CASE2-NEXT:    Symbol: foo
+# CASE2-NEXT:    Type:   R_X86_64_PC32
+# CASE2-NEXT: Symbols:
+# CASE2-NEXT:  - Name: foo
+# CASE2-NEXT:  - Name: 'foo [1]'
+  
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .text
+    Type: SHT_PROGBITS
+    Size: 8
+  - Name: .rela.text
+    Type: SHT_RELA
+    Info: .text
+    Link: .symtab
+    Relocations:
+      - Type: R_X86_64_PC32
+        Offset: 0
+        Symbol: 'foo [1]'
+      - Type: R_X86_64_PC32
+        Offset: 4
+        Symbol: foo
+Symbols:
+  - Name: foo
+  - Name: 'foo [1]'
+
+## Check obj2yaml does not add a suffix to a name if the
+## symbol is in .symtab and .dynsym at the same time.
+
+# RUN: yaml2obj --docnum=3 %s -o %t3
+# RUN: obj2yaml %t3 | FileCheck %s --check-prefix=CASE3
+
+# CASE3:      --- !ELF
+# CASE3-NEXT: FileHeader:
+# CASE3-NEXT:   Class:   ELFCLASS64
+# CASE3-NEXT:   Data:    ELFDATA2LSB
+# CASE3-NEXT:   Type:    ET_DYN
+# CASE3-NEXT:   Machine: EM_X86_64
+# CASE3-NEXT: Symbols:
+# CASE3-NEXT:   - Name:    foo
+# CASE3-NEXT:     Binding: STB_GLOBAL
+# CASE3-NEXT: DynamicSymbols:
+# CASE3-NEXT:   - Name:    foo
+# CASE3-NEXT:     Binding: STB_GLOBAL
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Symbols:
+  - Name:    foo
+    Binding: STB_GLOBAL
+DynamicSymbols:
+  - Name:    foo
+    Binding: STB_GLOBAL

llvm/trunk/test/tools/yaml2obj/duplicate-section-names.test

+## Check that yaml2obj is able to produce an object from YAML
+## containing sections with duplicate names (but different name suffixes).
+
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj -s %t1 | FileCheck %s --check-prefix=CASE1
+
+# CASE1: Name: .foo1 (
+# CASE1: Name: .foo (
+# CASE1: Name: .foo (
+# CASE1: Name: .foo2 (
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo1
+    Type: SHT_PROGBITS
+  - Name: .foo
+    Type: SHT_PROGBITS
+  - Name: '.foo [1]'
+    Type: SHT_PROGBITS
+  - Name: .foo2
+    Type: SHT_PROGBITS
+
+## Check that yaml2obj reports an error in case we have
+## sections with equal names and suffixes.
+
+# RUN: not yaml2obj --docnum=2 %s 2>&1 | FileCheck %s --check-prefix=CASE2
+# CASE2: error: Repeated section name: '.foo [1]' at YAML section number 1.
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: '.foo [1]'
+    Type: SHT_PROGBITS
+  - Name: '.foo [1]'
+    Type: SHT_PROGBITS
+
+## Check that yaml2obj reports an error in case we have
+## symbols without suffixes in the names and their
+## names are equal.
+
+# RUN: not yaml2obj --docnum=3 %s 2>&1 | FileCheck %s --check-prefix=CASE3
+# CASE3: error: Repeated section name: '.foo' at YAML section number 1.
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+  - Name: .foo
+    Type: SHT_PROGBITS
+
+## Check that yaml2obj can produce an object when symbols are defined
+## relative to sections with duplicate names (but different name suffixes).
+
+# RUN: yaml2obj --docnum=4 %s -o %t4
+# RUN: llvm-readobj -s -t %t4 | FileCheck %s --check-prefix=CASE4
+
+# CASE4:      Section {
+# CASE4:       Index: 1
+# CASE4-NEXT:  Name: .foo
+# CASE4:       Index: 2
+# CASE4-NEXT:  Name: .foo
+
+# CASE4:      Symbol {
+# CASE4:       Name: foo
+# CASE4-NEXT:  Value:
+# CASE4-NEXT:  Size:
+# CASE4-NEXT:  Binding:
+# CASE4-NEXT:  Type:
+# CASE4-NEXT:  Other:
+# CASE4-NEXT:  Section: .foo (0x1)
+# CASE4:       Name: bar
+# CASE4-NEXT:  Value:
+# CASE4-NEXT:  Size:
+# CASE4-NEXT:  Binding:
+# CASE4-NEXT:  Type:
+# CASE4-NEXT:  Other:
+# CASE4-NEXT:  Section: .foo (0x2)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+  - Name: '.foo [1]'
+    Type: SHT_PROGBITS
+Symbols:
+  - Name:    foo
+    Section: .foo
+  - Name:    bar
+    Section: '.foo [1]'
+
+## Check that yaml2obj can produce SHT_GROUP sections that
+## reference sections and symbols with name suffixes.
+
+# RUN: yaml2obj --docnum=5 %s -o %t5
+# RUN: llvm-readobj --elf-section-groups %t5 | FileCheck %s --check-prefix=CASE5
+
+# CASE5:      Groups {
+# CASE5-NEXT:   Group {
+# CASE5-NEXT:     Name: .group (1)
+# CASE5-NEXT:     Index: 1
+# CASE5-NEXT:     Link: 5
+# CASE5-NEXT:     Info: 1
+# CASE5-NEXT:     Type: COMDAT (0x1)
+# CASE5-NEXT:     Signature: foo
+# CASE5-NEXT:     Section(s) in group [
+# CASE5-NEXT:       .text.foo (2)
+# CASE5-NEXT:     ]
+# CASE5-NEXT:   }
+# CASE5-NEXT:   Group {
+# CASE5-NEXT:     Name: .group (1)
+# CASE5-NEXT:     Index: 3
+# CASE5-NEXT:     Link: 5
+# CASE5-NEXT:     Info: 2
+# CASE5-NEXT:     Type: COMDAT (0x1)
+# CASE5-NEXT:     Signature: foo
+# CASE5-NEXT:     Section(s) in group [
+# CASE5-NEXT:       .text.foo (4)
+# CASE5-NEXT:     ]
+# CASE5-NEXT:   }
+# CASE5-NEXT: }
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .group
+    Type: SHT_GROUP
+    Link: .symtab
+    Info: foo
+    Members:
+      - SectionOrType: GRP_COMDAT
+      - SectionOrType: .text.foo
+  - Name: .text.foo
+    Type: SHT_PROGBITS
+  - Name: '.group [1]'
+    Type: SHT_GROUP
+    Link: .symtab
+    Info: 'foo [1]'
+    Members:         
+      - SectionOrType: GRP_COMDAT
+      - SectionOrType: '.text.foo [1]'
+  - Name: '.text.foo [1]'
+    Type: SHT_PROGBITS
+Symbols:
+  - Name:    foo
+    Section: .text.foo
+  - Name:    'foo [1]'
+    Section: '.text.foo [1]'

llvm/trunk/test/tools/yaml2obj/duplicate-symbol-names.test

+## Check that yaml2obj is able to produce an object from YAML
+## containing symbols with duplicate names (but different name suffixes).
+
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj -t %t1 | FileCheck %s --check-prefix=CASE1
+
+# CASE1: Name: localfoo (1)
+# CASE1: Name: localfoo (1)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Symbols:
+  - Name: localfoo
+  - Name: 'localfoo [1]'
+
+## Check that yaml2obj reports an error when we have
+## symbols with equal names and suffixes.
+
+# RUN: not yaml2obj --docnum=2 %s 2>&1| FileCheck %s --check-prefix=CASE2
+# CASE2: error: Repeated symbol name: 'localfoo [1]'.
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Symbols:
+  - Name: 'localfoo [1]'
+  - Name: 'localfoo [1]'
+
+## Check that yaml2obj reports an error when we have
+## symbols without suffixes in the names and their
+## names are equal.
+
+# RUN: not yaml2obj --docnum=3 %s 2>&1| FileCheck %s --check-prefix=CASE3
+# CASE3: error: Repeated symbol name: 'localfoo'.
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Symbols:
+  - Name:    localfoo
+    Section: .text.foo.1
+  - Name:    localfoo
+    Section: .text.foo.2
+
+## Check that yaml2obj can produce correct relocations that
+## reference symbols with name suffixes.
+
+# RUN: yaml2obj --docnum=4 %s -o %t4
+# RUN: llvm-readobj -r --expand-relocs %t4 | FileCheck %s --check-prefix=CASE4
+
+# CASE4:      Relocations [
+# CASE4-NEXT:   Section {{.*}} .rela.text {
+# CASE4-NEXT:     Relocation {
+# CASE4-NEXT:       Offset: 0x0
+# CASE4-NEXT:       Type: R_X86_64_NONE
+# CASE4-NEXT:       Symbol: foo (1)
+# CASE4-NEXT:       Addend: 0x0
+# CASE4-NEXT:     }
+# CASE4-NEXT:     Relocation {
+# CASE4-NEXT:       Offset: 0x1
+# CASE4-NEXT:       Type: R_X86_64_NONE
+# CASE4-NEXT:       Symbol: foo (2)
+# CASE4-NEXT:       Addend: 0x0
+# CASE4-NEXT:     }
+# CASE4-NEXT:   }
+# CASE4-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .text
+    Type: SHT_PROGBITS
+  - Name: .rela.text
+    Type: SHT_RELA
+    Info: .text
+    Link: .symtab
+    Relocations:
+      - Offset: 0x0
+        Type:   R_X86_64_NONE
+        Symbol: foo
+      - Offset: 0x1
+        Type:   R_X86_64_NONE
+        Symbol: 'foo [1]'
+Symbols:
+  - Name: foo
+  - Name: 'foo [1]'

llvm/trunk/tools/obj2yaml/elf2yaml.cpp

class ELFDumper {
   typedef object::Elf_Sym_Impl<ELFT> Elf_Sym;
   typedef typename ELFT::Dyn Elf_Dyn;
   typedef typename ELFT::Shdr Elf_Shdr;
   typedef typename ELFT::Word Elf_Word;
   typedef typename ELFT::Rel Elf_Rel;
   typedef typename ELFT::Rela Elf_Rela;
 
   ArrayRef<Elf_Shdr> Sections;
+  ArrayRef<Elf_Sym> SymTable;
 
-  // If the file has multiple sections with the same name, we add a
-  // suffix to make them unique.
-  unsigned Suffix = 0;
-  DenseSet<StringRef> UsedSectionNames;
+  DenseMap<StringRef, uint32_t> UsedSectionNames;
   std::vector<std::string> SectionNames;
+
+  DenseMap<StringRef, uint32_t> UsedSymbolNames;
+  std::vector<std::string> SymbolNames;
+
   Expected<StringRef> getUniquedSectionName(const Elf_Shdr *Sec);
-  Expected<StringRef> getSymbolName(const Elf_Sym *Sym, StringRef StrTable,
+  Expected<StringRef> getUniquedSymbolName(const Elf_Sym *Sym,
+                                           StringRef StrTable,
                                            const Elf_Shdr *SymTab);
 
   const object::ELFFile<ELFT> &Obj;
   ArrayRef<Elf_Word> ShndxTable;
 
   std::error_code dumpSymbols(const Elf_Shdr *Symtab,
                               std::vector<ELFYAML::Symbol> &Symbols);
   std::error_code dumpSymbol(const Elf_Sym *Sym, const Elf_Shdr *SymTab,
                              StringRef StrTable, ELFYAML::Symbol &S);
ELFDumper<ELFT>::getUniquedSectionName(const Elf_Shdr *Sec) {
   if (!SectionNames[SecIndex].empty())
     return SectionNames[SecIndex];
 
   auto NameOrErr = Obj.getSectionName(Sec);
   if (!NameOrErr)
     return NameOrErr;
   StringRef Name = *NameOrErr;
   std::string &Ret = SectionNames[SecIndex];
+
+  auto It = UsedSectionNames.insert({Name, 0});
+  if (!It.second)
+    Ret = (Name + " [" + Twine(++It.first->second) + "]").str();
+  else
     Ret = Name;
-  while (!UsedSectionNames.insert(Ret).second)
-    Ret = (Name + to_string(++Suffix)).str();
   return Ret;
 }
 
 template <class ELFT>
-Expected<StringRef> ELFDumper<ELFT>::getSymbolName(const Elf_Sym *Sym,
-                                                   StringRef StrTable,
-                                                   const Elf_Shdr *SymTab) {
+Expected<StringRef>
+ELFDumper<ELFT>::getUniquedSymbolName(const Elf_Sym *Sym, StringRef StrTable,
   Expected<StringRef> SymbolNameOrErr = Sym->getName(StrTable);
   if (!SymbolNameOrErr)
     return SymbolNameOrErr;
   StringRef Name = *SymbolNameOrErr;
   if (Name.empty() && Sym->getType() == ELF::STT_SECTION) {
     auto ShdrOrErr = Obj.getSection(Sym, SymTab, ShndxTable);
     if (!ShdrOrErr)
       return ShdrOrErr.takeError();
     return getUniquedSectionName(*ShdrOrErr);
   }
+
+  // Symbols in .symtab can have duplicate names. For example, it is a common
+  // situation for local symbols in a relocatable object. Here we assign unique
+  // suffixes for such symbols so that we can differentiate them.
+  if (SymTab->sh_type == ELF::SHT_SYMTAB) {
+    unsigned Index = Sym - SymTable.data();
+    if (!SymbolNames[Index].empty())
+      return SymbolNames[Index];
+
+    auto It = UsedSymbolNames.insert({Name, 0});
+    if (!It.second)
+      SymbolNames[Index] =
+          (Name + " [" + Twine(++It.first->second) + "]").str();
+    else
+      SymbolNames[Index] = Name;
+    return SymbolNames[Index];
+  }
+
   return Name;
 }
 
 template <class ELFT> ErrorOr<ELFYAML::Object *> ELFDumper<ELFT>::dump() {
   auto Y = make_unique<ELFYAML::Object>();
 
   // Dump header
   Y->Header.Class = ELFYAML::ELF_ELFCLASS(Obj.getHeader()->getFileClass());
   Y->Header.Data = ELFYAML::ELF_ELFDATA(Obj.getHeader()->getDataEncoding());
   Y->Header.OSABI = Obj.getHeader()->e_ident[ELF::EI_OSABI];
   Y->Header.ABIVersion = Obj.getHeader()->e_ident[ELF::EI_ABIVERSION];
   Y->Header.Type = Obj.getHeader()->e_type;
   Y->Header.Machine = Obj.getHeader()->e_machine;
   Y->Header.Flags = Obj.getHeader()->e_flags;
   Y->Header.Entry = Obj.getHeader()->e_entry;
 
-  const Elf_Shdr *Symtab = nullptr;
-  const Elf_Shdr *DynSymtab = nullptr;
-
   // Dump sections
   auto SectionsOrErr = Obj.sections();
   if (!SectionsOrErr)
     return errorToErrorCode(SectionsOrErr.takeError());
   Sections = *SectionsOrErr;
   SectionNames.resize(Sections.size());
+
+  // Dump symbols. We need to do this early because other sections might want
+  // to access the deduplicated symbol names that we also create here.
+  for (const Elf_Shdr &Sec : Sections) {
+    if (Sec.sh_type == ELF::SHT_SYMTAB)
+      if (auto EC = dumpSymbols(&Sec, Y->Symbols))
+        return EC;
+    if (Sec.sh_type == ELF::SHT_DYNSYM)
+      if (auto EC = dumpSymbols(&Sec, Y->DynamicSymbols))
+        return EC;
+  }
+
   for (const Elf_Shdr &Sec : Sections) {
     switch (Sec.sh_type) {
     case ELF::SHT_DYNAMIC: {
       ErrorOr<ELFYAML::DynamicSection *> S = dumpDynamicSection(&Sec);
       if (std::error_code EC = S.getError())
         return EC;
       Y->Sections.push_back(std::unique_ptr<ELFYAML::Section>(S.get()));
       break;
     }
     case ELF::SHT_NULL:
     case ELF::SHT_STRTAB:
-      // Do not dump these sections.
-      break;
     case ELF::SHT_SYMTAB:
-      Symtab = &Sec;
-      break;
     case ELF::SHT_DYNSYM:
-      DynSymtab = &Sec;
+      // Do not dump these sections.
       break;
     case ELF::SHT_SYMTAB_SHNDX: {
       auto TableOrErr = Obj.getSHNDXTable(Sec);
       if (!TableOrErr)
         return errorToErrorCode(TableOrErr.takeError());
       ShndxTable = *TableOrErr;
       break;
     }
default: {
       ErrorOr<ELFYAML::RawContentSection *> S = dumpContentSection(&Sec);
       if (std::error_code EC = S.getError())
         return EC;
       Y->Sections.push_back(std::unique_ptr<ELFYAML::Section>(S.get()));
     }
     }
   }
 
-  if (auto EC = dumpSymbols(Symtab, Y->Symbols))
-    return EC;
-  if (auto EC = dumpSymbols(DynSymtab, Y->DynamicSymbols))
-    return EC;
-
   return Y.release();
 }
 
 template <class ELFT>
 std::error_code
 ELFDumper<ELFT>::dumpSymbols(const Elf_Shdr *Symtab,
                              std::vector<ELFYAML::Symbol> &Symbols) {
   if (!Symtab)
     return std::error_code();
 
   auto StrTableOrErr = Obj.getStringTableForSymtab(*Symtab);
   if (!StrTableOrErr)
     return errorToErrorCode(StrTableOrErr.takeError());
   StringRef StrTable = *StrTableOrErr;
 
   auto SymtabOrErr = Obj.symbols(Symtab);
   if (!SymtabOrErr)
     return errorToErrorCode(SymtabOrErr.takeError());
 
+  if (Symtab->sh_type == ELF::SHT_SYMTAB) {
+    SymTable = *SymtabOrErr;
+    SymbolNames.resize(SymTable.size());
+  }
+
   for (const auto &Sym : (*SymtabOrErr).drop_front()) {
     ELFYAML::Symbol S;
     if (auto EC = dumpSymbol(&Sym, Symtab, StrTable, S))
       return EC;
     Symbols.push_back(S);
   }
 
   return std::error_code();
 }
 
 template <class ELFT>
 std::error_code
 ELFDumper<ELFT>::dumpSymbol(const Elf_Sym *Sym, const Elf_Shdr *SymTab,
                             StringRef StrTable, ELFYAML::Symbol &S) {
   S.Type = Sym->getType();
   S.Value = Sym->st_value;
   S.Size = Sym->st_size;
   S.Other = Sym->st_other;
   S.Binding = Sym->getBinding();
 
-  Expected<StringRef> SymbolNameOrErr = getSymbolName(Sym, StrTable, SymTab);
+  Expected<StringRef> SymbolNameOrErr =
+      getUniquedSymbolName(Sym, StrTable, SymTab);
   if (!SymbolNameOrErr)
     return errorToErrorCode(SymbolNameOrErr.takeError());
   S.Name = SymbolNameOrErr.get();
 
   if (Sym->st_shndx >= ELF::SHN_LORESERVE) {
     if (Sym->st_shndx == ELF::SHN_XINDEX)
       return obj2yaml_error::not_implemented;
     S.Index = (ELFYAML::ELF_SHN)Sym->st_shndx;
std::error_code ELFDumper<ELFT>::dumpRelocation(const RelT *Rel,
   if (!StrTabSec)
     return errorToErrorCode(StrTabSec.takeError());
   auto StrTabOrErr = Obj.getStringTable(*StrTabSec);
   if (!StrTabOrErr)
     return errorToErrorCode(StrTabOrErr.takeError());
   StringRef StrTab = *StrTabOrErr;
 
   if (Sym) {
-    Expected<StringRef> NameOrErr = getSymbolName(Sym, StrTab, SymTab);
+    Expected<StringRef> NameOrErr = getUniquedSymbolName(Sym, StrTab, SymTab);
     if (!NameOrErr)
       return errorToErrorCode(NameOrErr.takeError());
     R.Symbol = NameOrErr.get();
   } else {
     // We have some edge cases of relocations without a symbol associated,
     // e.g. an object containing the invalid (according to the System V
     // ABI) R_X86_64_NONE reloc. Create a symbol with an empty name instead
     // of crashing.
ErrorOr<ELFYAML::Group *> ELFDumper<ELFT>::dumpGroup(const Elf_Shdr *Shdr) {
   auto SymOrErr = Obj.getSymbol(Symtab, Shdr->sh_info);
   if (!SymOrErr)
     return errorToErrorCode(SymOrErr.takeError());
   auto StrTabOrErr = Obj.getStringTableForSymtab(*Symtab);
   if (!StrTabOrErr)
     return errorToErrorCode(StrTabOrErr.takeError());
 
   Expected<StringRef> SymbolName =
-      getSymbolName(*SymOrErr, *StrTabOrErr, Symtab);
+      getUniquedSymbolName(*SymOrErr, *StrTabOrErr, Symtab);
   if (!SymbolName)
     return errorToErrorCode(SymbolName.takeError());
   S->Signature = *SymbolName;
 
   auto MembersOrErr = Obj.template getSectionContentsAsArray<Elf_Word>(Shdr);
   if (!MembersOrErr)
     return errorToErrorCode(MembersOrErr.takeError());
 

llvm/trunk/tools/yaml2obj/yaml2elf.cpp

bool ELFState<ELFT>::initImplicitHeader(ELFState<ELFT> &State,
   else if (SecName == ".dynstr")
     State.initStrtabSectionHeader(Header, SecName, State.DotDynstr, CBA,
                                   YAMLSec);
   else
     return false;
   return true;
 }
 
+static StringRef dropUniqueSuffix(StringRef S) {
+  size_t SuffixPos = S.rfind(" [");
+  if (SuffixPos == StringRef::npos)
+    return S;
+  return S.substr(0, SuffixPos);
+}
+
 template <class ELFT>
 bool ELFState<ELFT>::initSectionHeaders(ELFState<ELFT> &State,
                                         std::vector<Elf_Shdr> &SHeaders,
                                         ContiguousBlobAccumulator &CBA) {
   // Build a list of sections we are going to add implicitly.
   std::vector<StringRef> ImplicitSections;
   for (StringRef Name : State.implicitSectionNames())
     if (State.SN2I.get(Name) > Doc.Sections.size())
for (size_t I = 1; I < Doc.Sections.size() + ImplicitSections.size() + 1; ++I) {
     StringRef SecName =
         Sec ? Sec->Name : ImplicitSections[I - Doc.Sections.size() - 1];
     if (initImplicitHeader(State, CBA, SHeader, SecName, Sec))
       continue;
 
     assert(Sec && "It can't be null unless it is an implicit section. But all "
                   "implicit sections should already have been handled above.");
 
-    SHeader.sh_name = DotShStrtab.getOffset(SecName);
+    SHeader.sh_name = DotShStrtab.getOffset(dropUniqueSuffix(SecName));
     SHeader.sh_type = Sec->Type;
     if (Sec->Flags)
       SHeader.sh_flags = *Sec->Flags;
     SHeader.sh_addr = Sec->Address;
     SHeader.sh_addralign = Sec->AddressAlign;
 
     if (!Sec->Link.empty()) {
       unsigned Index;
for (const auto &Sym : Symbols) {
     Elf_Sym &Symbol = Ret[++I];
 
     // If NameIndex, which contains the name offset, is explicitly specified, we
     // use it. This is useful for preparing broken objects. Otherwise, we add
     // the specified Name to the string table builder to get its offset.
     if (Sym.NameIndex)
       Symbol.st_name = *Sym.NameIndex;
     else if (!Sym.Name.empty())
-      Symbol.st_name = Strtab.getOffset(Sym.Name);
+      Symbol.st_name = Strtab.getOffset(dropUniqueSuffix(Sym.Name));
 
     Symbol.setBindingAndType(Sym.Binding, Sym.Type);
     if (!Sym.Section.empty()) {
       unsigned Index;
       if (!SN2I.lookup(Sym.Section, Index)) {
         WithColor::error() << "Unknown section referenced: '" << Sym.Section
                            << "' by YAML symbol " << Sym.Name << ".\n";
         exit(1);
if (Section.Content)
     Section.Content->writeAsBinary(OS);
 
   return true;
 }
 
 template <class ELFT> bool ELFState<ELFT>::buildSectionIndex() {
   for (unsigned i = 0, e = Doc.Sections.size(); i != e; ++i) {
     StringRef Name = Doc.Sections[i]->Name;
-    DotShStrtab.add(Name);
+    DotShStrtab.add(dropUniqueSuffix(Name));
     // "+ 1" to take into account the SHT_NULL entry.
     if (!SN2I.addName(Name, i + 1)) {
       WithColor::error() << "Repeated section name: '" << Name
                          << "' at YAML section number " << i << ".\n";
       return false;
     }
   }
 
for (const auto &Sym : Symbols) {
     }
   }
   return true;
 }
 
 template <class ELFT> void ELFState<ELFT>::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
   for (const ELFYAML::Symbol &Sym : Doc.Symbols)
-    DotStrtab.add(Sym.Name);
+    DotStrtab.add(dropUniqueSuffix(Sym.Name));
   DotStrtab.finalize();
 
   // Add the dynamic symbol names to .dynstr section.
   for (const ELFYAML::Symbol &Sym : Doc.DynamicSymbols)
-    DotDynstr.add(Sym.Name);
+    DotDynstr.add(dropUniqueSuffix(Sym.Name));
 
   // SHT_GNU_verdef and SHT_GNU_verneed sections might also
   // add strings to .dynstr section.
   for (const std::unique_ptr<ELFYAML::Section> &Sec : Doc.Sections) {
     if (auto VerNeed = dyn_cast<ELFYAML::VerneedSection>(Sec.get())) {
       for (const ELFYAML::VerneedEntry &VE : VerNeed->VerneedV) {
         DotDynstr.add(VE.File);
         for (const ELFYAML::VernauxEntry &Aux : VE.AuxV)