Page MenuHomePhabricator

Handle section vs global name conflict.
ClosedPublic

Authored by eugenis on Mar 23 2016, 4:34 PM.

Details

Reviewers
pcc
rafael
Summary

This is a fix for PR26941.

When there is both a section and a global definition with the same
name, the global wins.

Section symbols are not added to the symbol table; section references
are left undefined and fixed up in the object writer unless they've
been satisfied by some other definition.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 51491.Mar 23 2016, 4:34 PM
eugenis retitled this revision from to Handle section vs global name conflict..
eugenis updated this object.
eugenis added reviewers: pcc, rafael.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.
pcc added inline comments.Mar 23 2016, 6:54 PM
include/llvm/MC/MCContext.h
103

I don't think you need to add this field. You could just use UsedNames for section names and use the bool value to keep track of whether the symbol name has been used for a non-section. The bool appears to be unused at the moment [1].

[1] http://llvm-cs.pcc.me.uk/include/llvm/MC/MCSymbol.h/rgetNameEntryPtr http://llvm-cs.pcc.me.uk/include/llvm/MC/MCSymbol.h/rgetNameEntryPtr-1

eugenis updated this revision to Diff 51595.Mar 24 2016, 1:32 PM
eugenis added inline comments.
include/llvm/MC/MCContext.h
103

Good point, done.

pcc added inline comments.Mar 24 2016, 1:47 PM
lib/MC/ELFObjectWriter.cpp
390

This problem affects the other object formats as well. Can't you do this in the generic object format lowering code with setVariableValue?

lib/MC/MCContext.cpp
184–190

You can simplify this to:

bool &Used = UsedNames[NewName];
if (!Used) {
  Used = true;
...
eugenis added inline comments.Mar 24 2016, 1:55 PM
lib/MC/ELFObjectWriter.cpp
390

In that case we emit two symbols instead of one. Also, the MCContext part is ELF specific, too.

eugenis added inline comments.Mar 24 2016, 2:01 PM
lib/MC/MCContext.cpp
184–190

MCSymbol constructor needs a pointer to StringMapEntry.

pcc added inline comments.Mar 24 2016, 2:26 PM
lib/MC/ELFObjectWriter.cpp
390

In that case we emit two symbols instead of one.

You could set IsTemporary on the symbol you look up.

Also, the MCContext part is ELF specific, too.

Sorry, I don't see how that is the case.

lib/MC/MCContext.cpp
184–190

Yes, sorry.

pcc added inline comments.Mar 24 2016, 2:42 PM
lib/MC/ELFObjectWriter.cpp
390

Okay, feel free to disregard all this. This does seem to be ELF-specific.

rafael accepted this revision.Mar 28 2016, 5:14 AM
rafael edited edge metadata.

LGTM with a nit.

test/MC/ELF/section-sym-redefine.s
24

Just to make it more complete, can you put symbol x4 in a section other than x4? Just adding

.section foo, "a", @progbits

before

x4:

should do it.

Also, please test a case where a symbol conflict with an implicit section. Name a symbol .text for example.

This revision is now accepted and ready to land.Mar 28 2016, 5:14 AM
eugenis updated this revision to Diff 51830.Mar 28 2016, 1:34 PM
eugenis edited edge metadata.
eugenis marked an inline comment as done.
eugenis closed this revision.Mar 28 2016, 1:42 PM

r264649, thanks for the review