[PATCH] [lld] Fix the ELF shared library build targets

Greg Fitzgerald garious at gmail.com
Thu Jan 22 15:06:22 PST 2015


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.


REPOSITORY
  rL LLVM

================
Comment at: include/lld/ReaderWriter/ELF/Targets.h:21
@@ +20,2 @@
+
+#endif
----------------
ruiu wrote:
> 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.
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.

================
Comment at: lib/Driver/GnuLdDriver.cpp:343
@@ +342,3 @@
+  default:
+    return nullptr;
+  }
----------------
ruiu wrote:
> Legitimate code should never reach here, no? If so, please use llvm_unreachable.
It can be reached by targeting an architecture that llvm supports but lld does not.

http://reviews.llvm.org/D7119

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list