Page MenuHomePhabricator

[lld-macho] Support __dso_handle for C++
ClosedPublic

Authored by int3 on Jul 10 2020, 6:26 PM.

Details

Reviewers
smeenai
Group Reviewers
Restricted Project
Commits
rG3587de228198: [lld-macho] Support __dso_handle for C++
Summary

The C++ ABI requires dylibs to pass a pointer to __cxa_atexit which does
e.g. cleanup of static global variables. The C++ spec says that the pointer
can point to any address in one of the dylib's segments, but in practice
ld64 seems to set it to point to the header, so that's what's implemented
here.

Diff Detail

Event Timeline

int3 created this revision.Jul 10 2020, 6:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2020, 6:26 PM
compnerd added inline comments.
lld/MachO/SymbolTable.cpp
101

Debugging left overs?

lld/MachO/Symbols.h
139

What do you think of making this ever so slightly more expensive? I think that we should actually make this computed - the name of the symbol is __dso_handle which is decorated with the user label prefix of _ to give you ___dso_handle. We have the information about the user label prefixes in LLVM, why not make that explicit here? I don't think that the cost is really that high.

int3 marked an inline comment as done.Jul 14 2020, 11:58 PM
int3 added inline comments.
lld/MachO/Symbols.h
139

I wasn't aware that LLVM had info about user label prefixes (though that makes sense). Happy to change. I suppose I'll want something like getTargetInfo().getDataLayout().getGlobalPrefix()

int3 marked an inline comment as done.Jul 15 2020, 12:10 AM
int3 added inline comments.
lld/MachO/Symbols.h
139

hmm DataLayout is under the IR folder though. I'm not sure how else to get at the user label, but we could probably just make it a global constant in lld-macho since the prefix applies to all macOS targets?

int3 updated this revision to Diff 278083.Jul 15 2020, 12:11 AM

remove fprintf

int3 marked an inline comment as done.Jul 15 2020, 12:11 AM
pcc added a subscriber: pcc.Jul 15 2020, 12:14 AM
pcc added inline comments.
lld/MachO/Symbols.h
121

I don't think the "C++ spec" says anything about this. If anything, it would be the Itanium C++ ABI.

139

FWIW, in general I'm not a fan of the idea of computing string constants like this, as it makes it harder to grep the code for strings like ___dso_handle.

int3 marked an inline comment as done.Jul 15 2020, 1:36 AM
int3 added inline comments.
lld/MachO/Symbols.h
121

yeah I wasn't super sure what the right names for these things were -- I thought the document describing the ABI would be considered a spec -- but I guess 'spec' connotes 'language spec'...

int3 marked an inline comment as done.Jul 15 2020, 12:28 PM
int3 added inline comments.
lld/MachO/Symbols.h
139

saw @pcc's comment later on, and I think it's a good reason to keep __dso_handle... also it's nice to be able to write the string inline in the class declaration rather than in the .cpp file.

int3 updated this revision to Diff 278281.Jul 15 2020, 12:32 PM

rephrase comment about ABI

compnerd added inline comments.Jul 16 2020, 10:51 AM
lld/MachO/Symbols.h
139

I'm suggesting that we keep __dso_handle which is still grepable, the prefix itself which is a single _ which is what I was suggesting that we remove. I suppose that its not the end of the world if we don't split it up, but using the IRMangler in LLVM is pretty common place and seems more precise/easier to understand to me.

int3 marked an inline comment as done.Jul 16 2020, 3:32 PM
int3 added inline comments.
lld/MachO/Symbols.h
139

I tried grepping for IRMangler and didn't find anything (and found a lot of results grepping for mangle). Which class were you thinking of?

pcc added inline comments.Jul 16 2020, 4:15 PM
lld/MachO/Symbols.h
139

If your starting point is ___dso_handle (for example, if you see that in nm output), it doesn't really help that __dso_handle is greppable.

I would also say that it makes the code harder to understand, by adding an unnecessary level of indirection. In order to understand what mangle("__dso_handle") does I would need to read the definition of mangle and establish that it always just prepends _ on Mach-O, so I know that it always returns "___dso_handle". But if I just see "___dso_handle" I get that knowledge straight away.

Anyway, I don't have much of a horse in this race, that's just my viewpoint.

MaskRay added inline comments.
lld/MachO/Symbols.h
139

I agree that having literal 3-underline ___dso_handle helps with grepping.

int3 added a comment.Jul 23 2020, 10:01 PM

Bump. Could this get a stamp?

smeenai accepted this revision.Jul 29 2020, 5:54 PM
smeenai added a subscriber: smeenai.
smeenai added inline comments.
lld/MachO/Symbols.h
139

I'm good with leaving the underscore in.

lld/test/MachO/dso-handle-no-override.s
4–8

What do you think about giving an explicit error vs. silently ignoring the user-defined symbol?

The local symbol thing is unfortunate, but I don't see a good way to handle it. (Plus I think symbols beginning with double underscores are supposed to be reserved for the implementation, so if a user has their own __dso_handle symbol they're in undefined territory anyway.)

This revision is now accepted and ready to land.Jul 29 2020, 5:54 PM
int3 added inline comments.Jul 30 2020, 10:52 AM
lld/test/MachO/dso-handle-no-override.s
4–8

Giving an explicit error is not much less complicated than discarding it in favor of __dso_handle -- we'd still need to make the createDefined function check that every symbol it's creating doesn't have that name. Not difficult, just ugly...

smeenai added inline comments.Jul 30 2020, 11:55 AM
lld/test/MachO/dso-handle-no-override.s
4–8

Not sure I follow? We're calling addDSOHandle after all the input files have been read (and their symbols added to the symbol table), correct? In that case, we'd just need to check that no symbol with that name already existed when we inserted it. Unless you meant to handle local symbols, in which case, I agree that seems ugly and probably not worth it.

int3 added inline comments.Jul 30 2020, 12:13 PM
lld/test/MachO/dso-handle-no-override.s
4–8

oh, I'd thought you were talking about the local-defined symbol. Yeah giving an explicit error for the global one wouldn't be a bad idea

int3 updated this revision to Diff 282040.Jul 30 2020, 1:57 PM

make duplicate definition an error

This revision was landed with ongoing or failed builds.Jul 30 2020, 2:30 PM
This revision was automatically updated to reflect the committed changes.