This is an archive of the discontinued LLVM Phabricator instance.

[lld] Fix the ELF shared library build targets
ClosedPublic

Authored by garious on Jan 21 2015, 10:01 PM.

Details

Summary

lldELF is used by each ELF backend. lldELF's ELFLinkingContext
also held a reference to each backend, creating a link-time cycle.
This patch moves the backend references to lldDriver.

Here's a link-time dependency diagram for what is implemented here: http://yuml.me/324beed8.

Diff Detail

Repository
rL LLVM

Event Timeline

garious updated this revision to Diff 18585.Jan 21 2015, 10:01 PM
garious retitled this revision from to [lld] Fix the ELF shared library build targets.
garious updated this object.
garious edited the test plan for this revision. (Show Details)
garious set the repository for this revision to rL LLVM.
garious added a project: lld.
garious added a subscriber: Unknown Object (MLST).

Could we keep the headers for each of the Target linking contexts inside each target itself.

lib/Driver/GnuLdDriver.cpp
19

Can you create a header file that contains all the elf targets. You need to sort all the includes without this though.

garious updated this revision to Diff 18631.Jan 22 2015, 11:35 AM

Per Shankar, tuck all ELF includes in one header that the Driver then includes.

Hi Shankar,

Could we keep the headers for each of the Target linking contexts inside each target itself.

I'm not sure exactly what you're asking for. Do you want the header
that describe's the component's public interface to stay close to
home? Or do you not want some other, presumably smaller, interface to
represent the public interface?

Thanks,
Greg

shankarke edited edge metadata.Jan 22 2015, 11:51 AM

Yes, could we move it close to where all the target specific files live ?

We could, but that would diverge from the way the LLVM repo is organized. I think a good compromise here would be to move all method implementations into each public header's respective .cpp file. Would you like me to do that as part of this patch?

I dont follow, llvm keeps target specific header files outside target directories ?

ruiu edited edge metadata.Jan 22 2015, 1:49 PM

In this patch you moved lib/ReaderWriter/ELF/<arch>/<arch>LinkingContext to lib/ReaderWriter/ELF. Why did you have to do that? Because of that change, the size of this diff is large, so it is not easy to find the answer myself. Could you elaborate that a bit?

include/lld/ReaderWriter/ELF/Targets.h
21 ↗(On Diff #18631)

I don't agree that this is a good idea. We should generally avoid transitive inclusions. If a source file depends on a bunch of headers, it actually depends on all of them. Hiding that dependency by replacing with one #include doesn't make things clean but makes things slightly worse.

lib/Driver/GnuLdDriver.cpp
343

Legitimate code should never reach here, no? If so, please use llvm_unreachable.

The reason I moved the headers is because the driver needs access each target's LinkingContext constructor. Before this patch, that code was in lldELF which created a cyclic dependency.

Another option is to leave the headers where they were and do what the LLVM build does, which is to generate a header with only the constructor definitions. Going that route will allow us to configure a build with some targets and not others. I'll take a stab at that and attempt to come back with a smaller patch.

include/lld/ReaderWriter/ELF/Targets.h
21 ↗(On Diff #18631)

It's a poor man's "<llvm>/include/llvm/Support/TargetSelect.h", which depends on the generated Targets.def. I can take a shot at implementing a similar header for LinkingContext constructors, but if that doesn't go well, I think this shim is the right short-term solution.

lib/Driver/GnuLdDriver.cpp
343

It can be reached by targeting an architecture that llvm supports but lld does not.

garious updated this revision to Diff 18642.Jan 22 2015, 4:46 PM
garious edited edge metadata.

This version leaves the targets' LinkingContext headers in their respective source directories and instead follows the pattern from the LLVM repo, which is to generate a header from the build configuration.

This revision is now accepted and ready to land.Jan 22 2015, 5:19 PM

Rui, any objections?

ruiu accepted this revision.Jan 23 2015, 10:39 AM
ruiu edited edge metadata.

Ah, no, sorry. LGTM.

garious updated this revision to Diff 18693.Jan 23 2015, 2:57 PM
garious edited edge metadata.

This version constructs the LinkingContext with static methods and a class factory. The previous version would blow up if a LinkingContext contained member variables.

Also, moved the triple test into their respective targets so that class factory registration could be generated from the configured targets. The registration is currently done by hand because:

  • PPCLinkingContext needs to be renamed to PowerPCLinkingContext
  • X86LinkingContext should construct other X86 or X86_64 context objects
  • Need to add 'create' methods for all the unimplemented architectures

These changes are all easy to make, but to keep this patch readable, is not done here.

Note: the "if elseif elseif" code I added to the driver is ugly, but after the above changes, will be deleted. There's lots of ways to implement that bit of code and I'm hoping we can discuss none of them at this time. :)

shankarke accepted this revision.Jan 23 2015, 4:00 PM
shankarke edited edge metadata.
ruiu added a comment.Jan 23 2015, 4:45 PM

LGTM

lib/Driver/GnuLdDriver.cpp
328

This else-ifs can be early if-returns. s/else if/if/g

lib/ReaderWriter/ELF/AArch64/AArch64LinkingContext.cpp
21

Remove this line

lib/ReaderWriter/ELF/ARM/ARMLinkingContext.cpp
22

Remove this blank line

lib/ReaderWriter/ELF/Hexagon/HexagonLinkingContext.cpp
20

Ditto

lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp
24

Ditto

lib/ReaderWriter/ELF/PPC/PPCLinkingContext.cpp
23

Ditto

lib/ReaderWriter/ELF/X86/X86LinkingContext.cpp
23

Ditto

lib/ReaderWriter/ELF/X86_64/X86_64LinkingContext.cpp
21

Ditto

garious closed this revision.Jan 23 2015, 5:12 PM

Should be fixed in r226989.

lib/ReaderWriter/ELF/X86_64/X86_64LinkingContext.cpp