Skip to content

Simplify DSO registration with druntime#2870

Merged
kinke merged 3 commits intoldc-developers:masterfrom
kinke:register_dso
Oct 16, 2018
Merged

Simplify DSO registration with druntime#2870
kinke merged 3 commits intoldc-developers:masterfrom
kinke:register_dso

Conversation

@kinke
Copy link
Member

@kinke kinke commented Oct 5, 2018

  • The explicit reference to the ModuleInfo should be obsolete, it's added to the llvm.used array.
  • Only keep a single pair of ldc.dso_{c,d}tor per DSO (pointing to a common ldc.register_dso() function) instead of a pair per linked-in D object file. This simplifies ldc.register_dso() and allows to get rid of the explicit ctor/dtor functions and the ldc.dso_initialized global.

@kinke
Copy link
Member Author

kinke commented Oct 5, 2018

Number of global {c,d}tor pairs for a Phobos hello-world with LDC v1.11: 143
With this: 2

=> Eliding 141 useless ctor/dtor calls on startup/shutdown & 2x142 functions (explicit {c,d}tor functions). The asm/IR output for trivial test cases will also be much nicer (147 vs. 211 lines for a printf hello-world -output-s).

@kinke
Copy link
Member Author

kinke commented Oct 5, 2018

Hmm, Travis' ld v2.24 apparently strips the ModuleInfos; CircleCI (also based on Ubuntu 14.04) works with gold.

Mac is looking bad too (well, when linking against static druntime/Phobos, fine with shared ones).

@kinke kinke changed the title Simplify DSO registration with druntime WIP: Simplify DSO registration with druntime Oct 5, 2018
@dnadlinger
Copy link
Member

Yep, as per the comments, a lot of the complexity comes from --gc-sections.

@kinke
Copy link
Member Author

kinke commented Oct 6, 2018

It's working with Ubuntu 18.04's ld (bfd) v2.30 at least, otherwise I wouldn't have opened this PR, so older versions of ld not respecting llvm.used may likely be a bug (and appears to be workable around by using gold).

Do you know if ld64 is doing something similar implicitly? AFAIK we only use --gc-sections for Linux targets.

@kinke
Copy link
Member Author

kinke commented Oct 6, 2018

Btw how would you guys feel about dropping LLVM support for < 3.9? IIRC, that's the minimum version required for MSVC and LTO, and there are 61 occurrences of #if LDC_LLVM_VER >= 309 in our C++ code.

@kinke
Copy link
Member Author

kinke commented Oct 6, 2018

AFAICT, the problem for macOS is that the global {c,d}tors aren't folded to a single pair. From SSH session:

$ size -x -l -m hello
...
Segment __DATA: 0x49000 (vmaddr 0x100235000 fileoff 2314240)
	...
	Section __mod_init_func: 0x480 (addr 0x10023bb60 offset 2341728) // ctors
	Section __mod_term_func: 0x480 (addr 0x10023bfe0 offset 2342880) // dtors
	...
	Section .minfo: 0x480 (addr 0x10027b468 offset 2602088) // ModuleInfos

So there are 0x480 / 8 = 144 ModuleInfos, and unfortunately the same number of ctors/dtors, while there should be a single one for those. As each _d_dso_registry() call toggles between registering and unregistering the DSO, it should be 72x registered and 72x unregistered at startup...

[The symptom is quite a different one, an LDC segfault in StringTable.update() when registering the predefined versions - if LDC is built with itself.]

@kinke
Copy link
Member Author

kinke commented Oct 6, 2018

Further findings: the 'workaround' leads to the same symptom on Linux, apparently because the manual {c,d}tor refs are stripped - makes sense. So I added them to llvm.used => ending up with all ctors again. So I put them into a Comdat - and that finally works again. But Mach-O of course doesn't support them, so I have no idea how to make this simpler scheme work on Mac.

@kinke kinke force-pushed the register_dso branch 3 times, most recently from 17f1301 to 3b14f9c Compare October 6, 2018 21:51
@kinke
Copy link
Member Author

kinke commented Oct 6, 2018

Emitting the refs manually works for macOS, they are folded correctly. Without putting them into llvm.used, they get stripped when using -O (I don't know where that comes from). Luckily, Comdats appear not to be required to prevent duplicate refs then.

@JohanEngelen
Copy link
Member

Btw how would you guys feel about dropping LLVM support for < 3.9?

Fine with me. LLVM 3.9 was released 2 years ago. (it's perhaps nice that we support so many LLVM versions, but it is a burden and I don't know how many people actually make use of that; clang only supports one LLVM version ;-) )

@kinke kinke force-pushed the register_dso branch 2 times, most recently from d64e005 to d1e9309 Compare October 6, 2018 23:09
@kinke kinke changed the title WIP: Simplify DSO registration with druntime Simplify DSO registration with druntime Oct 7, 2018
kinke added 3 commits October 15, 2018 22:33
* The explicit reference to the ModuleInfo should be obsolete, it's
  added to the llvm.used array.
* Only keep a single pair of ldc.dso_{c,d}tor per DSO (pointing to a
  common ldc.register_dso() function) instead of a pair per linked-in D
  object file. This simplifies ldc.register_dso() and allows to get rid
  of the explicit ctor/dtor functions and the ldc.dso_initialized global.
@kinke kinke merged commit c41cba2 into ldc-developers:master Oct 16, 2018
@kinke kinke deleted the register_dso branch October 16, 2018 00:19
@JohanEngelen
Copy link
Member

JohanEngelen commented Nov 11, 2018

We should write down clearer in the release notes why the default linker is now ld.gold and should very clearly write down that using another linker may break the binary (which is what I read here). Do I understand it correctly that this PR doesn't just set the default to ld.gold, it actually makes ld.gold a requirement? ld.gold is not always available, so in that case people cannot use LDC anymore?
The Ubuntu Trusty package gcc-aarch64-linux-gnu for cross compiling to AArch64 only contains ld.bfd 2.24. It's confusing to see the error message collect2: fatal error: cannot find 'ld' because of the now explicit fuse-ld=gold passed to gcc. The fix -linker=bfd is not obvious. Worse: does it break binaries?

@dnadlinger
Copy link
Member

Worse: does it break binaries?

If that's the case, worse indeed! Part of what made this difficult to develop in the first instance is precisely that users expect a compiler to just work with "the linker"; probably not many are aware that there are different linkers, or if they are, that there are differences beyond linking speed.

@kinke
Copy link
Member Author

kinke commented Nov 11, 2018

Worse: does it break binaries?

It does under following circumstances OTOH:

  • working around missing gold with -linker=bfd
  • bfd is an apparently buggy older one, stripping the ModuleInfos
  • ModuleInfos are required, e.g., to invoke module ctors

It's confusing to see the error message collect2: fatal error: cannot find 'ld' because of the now explicit fuse-ld=gold passed to gcc.

[What makes it worse is that the error msg doesn't say cannot find ld.gold, which would make it easier to track down.]


To me, the advantages of this PR are clear (excl. the switch to gold, i.e., the codegen improvements). If older bfd really ignores llvm.used (whatever that lowers down to in asm), then it's not just an issue for ModuleInfos, but for the @assumeUsed UDA too. If we still have to support it, I'd prefer looking into the bfd issue and find a workaround for it, instead of going the quick'n dirty route of (temporarily) reverting this PR.

[I assumed gold would be part of all gcc/binutils packages.]

@kinke
Copy link
Member Author

kinke commented Nov 11, 2018

The Ubuntu Trusty package gcc-aarch64-linux-gnu for cross compiling to AArch64 only contains ld.bfd 2.24.

gold is included in the binutils-aarch64-linux-gnu packages for xenial (16.04) and bionic (18.04).

@kinke
Copy link
Member Author

kinke commented Nov 11, 2018

Debian also includes gold in its oldest supported binutils-aarch64-linux-gnu/binutils-arm-linux-gnueabihf packages (Jessie; binutils-2.25).

@kinke
Copy link
Member Author

kinke commented Nov 11, 2018

Trusty's binutils-arm-linux-gnueabi does include gold as well (as does the native armhf package).

It's missing in AArch64's native binutils package too though (we're still talking about Trusty). It does look like an isolated special case to me though.

Edit: I checked all binutils Trusty packages - arm64 is the only one missing gold, i.e., PowerPC etc. all include it. I also checked Arch and Fedora (recent ones, x86_64 only), they include it; I couldn't quickly find a file listing for the Suse package.

@kinke
Copy link
Member Author

kinke commented Nov 11, 2018

So unless there are other prominent distros missing gold, I'd say this is fine, considering Trusty reaches EOL in half a year anyway. gold being significantly faster and preventing LTO issues makes it a rather good default on Linux IMO.

I'm not sure mentioning that an explicit -linker=bfd may break existing code in combination with an old bfd is worth it. It's a single sentence, but still, I keep on being surprised by who doesn't read the GitHub release notes, to the point of wondering who actually does. My aim was and is to keep them as tight as possible, so that people don't feel overwhelmed and turned off, but it's apparently not a sufficient recipe. ;)

@JohanEngelen
Copy link
Member

It is very nice that you did the digging to get data about availability of ld.gold, thanks.
I still think that it's better to change the wording to something like "due to a bug in the bfd linker, ld.gold is a now requirement on Linux, which can be worked around using -linker ".
I do read the release notes (often several versions back)! I find them more useful for reference (when experiencing troubles after updating the compiler) than for news flash bits. (it's a compiler after all)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants