[PATCH] D83603: [lld-macho] Support __dso_handle for C++

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/Symbols.h:139
+
+  static const constexpr StringRef name = "___dso_handle";
+
----------------
MaskRay wrote:
> pcc wrote:
> > int3 wrote:
> > > compnerd wrote:
> > > > int3 wrote:
> > > > > int3 wrote:
> > > > > > int3 wrote:
> > > > > > > pcc wrote:
> > > > > > > > compnerd wrote:
> > > > > > > > > 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.
> > > > > > > > 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`.
> > > > > > > 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()`
> > > > > > 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?
> > > > > 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.
> > > > 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.
> > > 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?
> > 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.
> I agree that having literal 3-underline `___dso_handle` helps with grepping.
I'm good with leaving the underscore in.


================
Comment at: lld/test/MachO/dso-handle-no-override.s:3-7
+## If for some bizarre reason the input file defines its own ___dso_handle, we
+## should ignore it. At least, we've implemented this behavior if the
+## conflicting symbol is a global. A local symbol of the same name will still
+## take priority in our implementation, unlike in ld64. But that's a pretty
+## far-out edge case that should be safe to ignore.
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83603