Page MenuHomePhabricator

Introduce CommonSymbol.
ClosedPublic

Authored by ruiu on Tue, May 14, 6:25 AM.

Details

Summary

Previously, we handled common symbols as a kind of Defined symbol,
but what we were doing for common symbols is pretty different from
regular defined symbols.

Common symbol and defined symbol are probably as different as shared
symbol and defined symbols are different.

This patch introduces CommonSymbol to represent common symbols.
After symbols are resolved, they are converted to Defined symbols
residing in a .bss section.

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu created this revision.Tue, May 14, 6:25 AM
Herald added a project: Restricted Project. · View Herald Transcript

This is interesting, could you please hold this on (in case you'll have LGTM from other person) until I'll look more carefully in my tomorrow (+24h)?

This is interesting, could you please hold this on (in case you'll have LGTM from other person) until I'll look more carefully in my tomorrow (+24h)?

You beat me to the draw:) I wanted to say the same. I also need a careful analysis of this...

MaskRay added inline comments.Wed, May 15, 12:25 AM
lld/ELF/SymbolTable.cpp
155 ↗(On Diff #199422)

mergeProperties(Old, New); can be moved after if (Old->isPlaceholder()) {

See my question in D61855. I think we should decrease the number of redundant assignments (if possible).

156 ↗(On Diff #199422)

Not the fault of this patch, but this comment looks obscure..

isa<SharedSymbol>(Old) -> Old->isShared().

496 ↗(On Diff #199422)

getDemangledSyms is transitively called by scanVersioScript before replaceCommonSymbols. Is a isCommon() check needed here?

511 ↗(On Diff #199422)

Ditto. Is a isCommon() check needed here?

528 ↗(On Diff #199422)

Ditto. Is a isCommon() check needed here?

grimar added inline comments.Wed, May 15, 4:16 AM
lld/ELF/SymbolTable.cpp
136 ↗(On Diff #199422)

Missing comment.

295 ↗(On Diff #199422)

Not strong opinion, but perhaps !OldSym->Section && !NewSym->Section would be better (while you are here).

320 ↗(On Diff #199422)

Lets use CommonSymbol instead of auto? OldSym has an obvious type here, but I feel sometimes that auto is something
that was invented for iterators or stuff like that and often used a bit too often than needed.

I.e. I think it should be used when the type is so complex in spelling that you should not think about it. Otherwise for readablility,
like in this place, probably it is better just to use the original type name. What do you think?

lld/ELF/Symbols.h
254 ↗(On Diff #199422)

delcarations -> declarations
Neverthless -> Nevertheless
varaible -> variable

(actually this is my spell checker who complains, I was fine with the original version :])

ruiu marked an inline comment as done.Wed, May 15, 5:40 AM

Looks like it is not easy to make changes to a patch in a patch series as it causes merge conflicts. It's still better than merging them into one big patch and send it for review, but splitting a patch makes my life a bit harder when it comes to updating it. Do you have any critical comments that need to be addressed? I'll make changes as you guys requested before committing, so if this patch looks generally OK, could you stamp on it for me?

lld/ELF/SymbolTable.cpp
496 ↗(On Diff #199422)

Yes, I guess so. I'll do that.

grimar accepted this revision.Wed, May 15, 6:17 AM

Generally it looks good to me.

This revision is now accepted and ready to land.Wed, May 15, 6:17 AM
ruiu added a comment.Wed, May 15, 6:17 AM

Thanks! I'll commit this tomorrow.

MaskRay accepted this revision.Wed, May 15, 6:52 AM
MaskRay added inline comments.
lld/ELF/Driver.cpp
1697 ↗(On Diff #199422)

regular defined symbols?

lld/ELF/Symbols.h
238 ↗(On Diff #199422)

The comment may mention this is for C tentative definitions (its origin is Fortran COMMON blocks, which may predate Unix...)

ruiu marked 2 inline comments as done.Wed, May 15, 8:23 PM
ruiu added inline comments.
lld/ELF/SymbolTable.cpp
155 ↗(On Diff #199422)

Currently the first symbol needs to be merged with itself. I'll try to fix later.

320 ↗(On Diff #199422)

I'm fine in either way.

Closed by commit rL360841: Introduce CommonSymbol. (authored by ruiu, committed by ). · Explain WhyWed, May 15, 8:26 PM
This revision was automatically updated to reflect the committed changes.