Page MenuHomePhabricator

[ELF] Define __dso_handle symbol if needed
ClosedPublic

Authored by phosek on Jun 2 2017, 5:02 PM.

Details

Summary

Traditionally, it has been defined in crtbegin.o, which is typically provided by libgcc or as part of the C library on some systems. However, but there's no principled reason for it to be there. We optionally define this symbol, which can be used on platforms that don't provide __dso_handle in crtbegin.o or which don't use crtbegin.o at all.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Jun 2 2017, 5:02 PM
tpimh added a subscriber: tpimh.Jun 3 2017, 12:16 AM
ruiu edited edge metadata.Jun 5 2017, 7:14 AM

Overall looks fine. I think this is the right approach to define __dso_handle, as it is very easy to do, compared to other solutions.

ELF/Writer.cpp
913 ↗(On Diff #101295)

I'd describe a bit more about the __dso_handle symbol so that the code is more self-contained for the first-time reader of the source code. Specifically, I'd like to mention

  • dso_handle is passed to cxa_finalize as a marker to identify each DSO
  • the address of the symbol doesn't matter as long as they are different in different DSOs, so we chose the start address of the DSO

In particular, "... with a value which is an address in one of the object's segment" sounds too formal specification-ish. I'd make it more concrete.

916 ↗(On Diff #101295)

Don't you want to do this only when Config->Shared?

phosek updated this revision to Diff 101443.Jun 5 2017, 12:37 PM
phosek marked an inline comment as done.
phosek added inline comments.Jun 5 2017, 12:47 PM
ELF/Writer.cpp
916 ↗(On Diff #101295)

The frontend generates the __dso_handle reference in any case because it doesn't whether the code is going to be linked into a shared library or not so we need to generate this symbol even in the non-shared case.

ruiu accepted this revision.Jun 5 2017, 1:09 PM

LGTM

This revision is now accepted and ready to land.Jun 5 2017, 1:09 PM
ruiu added inline comments.Jun 5 2017, 1:10 PM
ELF/Writer.cpp
910 ↗(On Diff #101443)

Remove an extra space.

phosek updated this revision to Diff 101454.Jun 5 2017, 1:42 PM
phosek marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.