<div dir="ltr">Whoops, update one of the testcases to make sure it actually makes.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 21, 2015 at 5:31 PM, Luqman Aden <span dir="ltr"><<a href="mailto:me+llvm@luqman.ca" target="_blank">me+llvm@luqman.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, Apr 21, 2015 at 3:42 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br>
> On 2015-Apr-20, at 15:07, Luqman Aden <<a href="mailto:me%2Bllvm@luqman.ca" target="_blank">me+llvm@luqman.ca</a>> wrote:<br>
><br>
><br>
><br>
> On Mon, Apr 20, 2015 at 1:59 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
> > On 2015-Apr-20, at 13:28, Luqman Aden <<a href="mailto:me%2Bllvm@luqman.ca" target="_blank">me+llvm@luqman.ca</a>> wrote:<br>
> ><br>
> > From df1b11c561dc826d8e4836b9e910ac75185bcf05 Mon Sep 17 00:00:00 2001<br>
> > From: Luqman Aden <<a href="mailto:luqman@apple.com" target="_blank">luqman@apple.com</a>><br>
> > Date: Wed, 15 Apr 2015 12:22:32 -0700<br>
> > Subject: [PATCH] llvm-link: Add -override flag to prefer one of a duplicate<br>
> > symbol.<br>
> ><br>
> > ---<br>
> > include/llvm/Linker/Linker.h | 4 +++-<br>
> > lib/Linker/LinkModules.cpp | 20 +++++++++++++++----<br>
> > test/tools/llvm-link/Inputs/foo.ll | 4 ++++<br>
> > test/tools/llvm-link/hello.ll | 20 +++++++++++++++++++<br>
> > test/tools/llvm-link/lit.local.cfg | 1 +<br>
> > tools/llvm-link/llvm-link.cpp | 40 ++++++++++++++++++++++++--------------<br>
> > 6 files changed, 69 insertions(+), 20 deletions(-)<br>
> > create mode 100644 test/tools/llvm-link/Inputs/foo.ll<br>
> > create mode 100644 test/tools/llvm-link/hello.ll<br>
> > create mode 100644 test/tools/llvm-link/lit.local.cfg<br>
> ><br>
><br>
> > From df1b11c561dc826d8e4836b9e910ac75185bcf05 Mon Sep 17 00:00:00 2001<br>
> > From: Luqman Aden <<a href="mailto:luqman@apple.com" target="_blank">luqman@apple.com</a>><br>
> > Date: Wed, 15 Apr 2015 12:22:32 -0700<br>
> > Subject: [PATCH] llvm-link: Add -override flag to prefer one of a duplicate<br>
> > symbol.<br>
><br>
> Luqman and I talked in person, and he added some context for this (seems<br>
> to still be missing from llvm-commits?). The idea is to support an LLVM<br>
> developer workflow where a subset of the module is extracted to a side<br>
> module to be manipulated or optimized differently. Sometime before<br>
> CodeGen, these functions are merged back in, and should replace the<br>
> copies in the "main" module (regardless of IR linkage types).<br>
><br>
> This seems like a pretty useful debugging workflow to me, and the<br>
> approach in this patch seems about right.<br>
><br>
> @Rafael, I'd appreciate your opinion on this.<br>
><br>
> ><br>
> > ---<br>
> > include/llvm/Linker/Linker.h | 4 +++-<br>
> > lib/Linker/LinkModules.cpp | 20 +++++++++++++++----<br>
> > test/tools/llvm-link/Inputs/foo.ll | 4 ++++<br>
> > test/tools/llvm-link/hello.ll | 20 +++++++++++++++++++<br>
> > test/tools/llvm-link/lit.local.cfg | 1 +<br>
> > tools/llvm-link/llvm-link.cpp | 40 ++++++++++++++++++++++++--------------<br>
> > 6 files changed, 69 insertions(+), 20 deletions(-)<br>
> > create mode 100644 test/tools/llvm-link/Inputs/foo.ll<br>
> > create mode 100644 test/tools/llvm-link/hello.ll<br>
> > create mode 100644 test/tools/llvm-link/lit.local.cfg<br>
> ><br>
> > diff --git a/include/llvm/Linker/Linker.h b/include/llvm/Linker/Linker.h<br>
> > index 5ca815c..c43b90e 100644<br>
> > --- a/include/llvm/Linker/Linker.h<br>
> > +++ b/include/llvm/Linker/Linker.h<br>
> > @@ -68,8 +68,10 @@ public:<br>
> > void deleteModule();<br>
> ><br>
> > /// \brief Link \p Src into the composite. The source is destroyed.<br>
> > + /// Passing OverrideSymbols as true will have symbols from Src<br>
> > + /// shadow those in the Dest.<br>
> > /// Returns true on error.<br>
> > - bool linkInModule(Module *Src);<br>
> > + bool linkInModule(Module *Src, bool OverrideSymbols = false);<br>
> ><br>
> > /// \brief Set the composite to the passed-in module.<br>
> > void setModule(Module *Dst);<br>
> > diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp<br>
> > index 21edc50..3a46a33 100644<br>
> > --- a/lib/Linker/LinkModules.cpp<br>
> > +++ b/lib/Linker/LinkModules.cpp<br>
> > @@ -424,12 +424,17 @@ class ModuleLinker {<br>
> ><br>
> > DiagnosticHandlerFunction DiagnosticHandler;<br>
> ><br>
> > + /// For symbol clashes, prefer those from Src.<br>
> > + bool OverrideFromSrc;<br>
> > +<br>
> > public:<br>
> > ModuleLinker(Module *dstM, Linker::IdentifiedStructTypeSet &Set, Module *srcM,<br>
> > - DiagnosticHandlerFunction DiagnosticHandler)<br>
> > + DiagnosticHandlerFunction DiagnosticHandler,<br>
> > + bool Override = false)<br>
><br>
> Why spell this differently from the class member?<br>
><br>
> Also, are there any callers that don't specify this flag? If not, don't<br>
> provide a default.<br>
><br>
> Updated.<br>
><br>
><br>
> > : DstM(dstM), SrcM(srcM), TypeMap(Set),<br>
> > ValMaterializer(TypeMap, DstM, LazilyLinkGlobalValues),<br>
> > - DiagnosticHandler(DiagnosticHandler) {}<br>
> > + DiagnosticHandler(DiagnosticHandler),<br>
> > + OverrideFromSrc(Override) {}<br>
> ><br>
> > bool run();<br>
> ><br>
> > @@ -725,6 +730,12 @@ bool ModuleLinker::getComdatResult(const Comdat *SrcC,<br>
> > bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,<br>
> > const GlobalValue &Dest,<br>
> > const GlobalValue &Src) {<br>
> > + // Should we unconditionally use the Src?<br>
> > + if (OverrideFromSrc) {<br>
> > + LinkFromSrc = true;<br>
> > + return false;<br>
> > + }<br>
> > +<br>
> > // We always have to add Src if it has appending linkage.<br>
> > if (Src.hasAppendingLinkage()) {<br>
> > LinkFromSrc = true;<br>
> > @@ -794,6 +805,7 @@ bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,<br>
> > assert(!Dest.hasExternalWeakLinkage());<br>
> > assert(Dest.hasExternalLinkage() && Src.hasExternalLinkage() &&<br>
> > "Unexpected linkage type!");<br>
> > +<br>
><br>
> This seems unrelated to the patch?<br>
><br>
> Yes, whitespace left over from debugging before. I've now removed it.<br>
><br>
><br>
> > return emitError("Linking globals named '" + Src.getName() +<br>
> > "': symbol multiply defined!");<br>
> > }<br>
> > @@ -1742,9 +1754,9 @@ void Linker::deleteModule() {<br>
> > Composite = nullptr;<br>
> > }<br>
> ><br>
> > -bool Linker::linkInModule(Module *Src) {<br>
> > +bool Linker::linkInModule(Module *Src, bool OverrideSymbols) {<br>
> > ModuleLinker TheLinker(Composite, IdentifiedStructTypes, Src,<br>
> > - DiagnosticHandler);<br>
> > + DiagnosticHandler, OverrideSymbols);<br>
> > bool RetCode = TheLinker.run();<br>
> > Composite->dropTriviallyDeadConstantArrays();<br>
> > return RetCode;<br>
> > diff --git a/test/tools/llvm-link/Inputs/foo.ll b/test/tools/llvm-link/Inputs/foo.ll<br>
> > new file mode 100644<br>
> > index 0000000..6e06fa5<br>
> > --- /dev/null<br>
> > +++ b/test/tools/llvm-link/Inputs/foo.ll<br>
> > @@ -0,0 +1,4 @@<br>
> > +define i32 @foo(i32 %i) {<br>
> > +entry:<br>
> > + ret i32 4<br>
> > +}<br>
><br>
> We usually put lib/Linker tests in "test/Linker".<br>
><br>
> > diff --git a/test/tools/llvm-link/hello.ll b/test/tools/llvm-link/hello.ll<br>
> > new file mode 100644<br>
> > index 0000000..c124864<br>
> > --- /dev/null<br>
> > +++ b/test/tools/llvm-link/hello.ll<br>
> > @@ -0,0 +1,20 @@<br>
> > +; RUN: llvm-as %s -o %t-hello.bc<br>
> > +; RUN: llvm-as %p/Inputs/foo.ll -o %t-foo.bc<br>
><br>
> Why not just use llvm-link directly?<br>
><br>
> I initially thought it wanted .bc only and just hadn't updated the test there.<br>
><br>
><br>
> Also, I recommend this naming scheme:<br>
><br>
> test/Linker/manual-override.ll<br>
> test/Linker/Inputs/manual-override.ll<br>
><br>
> (Or whatever you want to name the tests, but I wouldn't call it "hello".)<br>
><br>
> I'd also use `%S` instead of `%p`, since last I checked only the<br>
> former is actually documented at <a href="http://llvm.org" target="_blank">llvm.org</a>.<br>
><br>
> Also, it might be worth testing both command-line orderings.<br>
><br>
> Added.<br>
><br>
><br>
> > +; RUN: llvm-link %t-hello.bc -override %t-foo.bc -S | FileCheck %s<br>
> > +<br>
> > +<br>
> > +; CHECK-LABEL: define i32 @foo<br>
> > +; CHECK-NEXT: entry:<br>
> > +; CHECK-NEXT: ret i32 4<br>
> > +define i32 @foo(i32 %i) {<br>
> > +entry:<br>
> > + %add = add nsw i32 %i, %i<br>
> > + ret i32 %add<br>
> > +}<br>
> > +<br>
> > +; Function Attrs: nounwind ssp uwtable<br>
> > +define i32 @main(i32 %argc, i8** %argv) {<br>
> > +entry:<br>
> > + %a = call i32 @foo(i32 2)<br>
> > + ret i32 %a<br>
> > +}<br>
> > diff --git a/test/tools/llvm-link/lit.local.cfg b/test/tools/llvm-link/lit.local.cfg<br>
> > new file mode 100644<br>
> > index 0000000..c6106e4<br>
> > --- /dev/null<br>
> > +++ b/test/tools/llvm-link/lit.local.cfg<br>
> > @@ -0,0 +1 @@<br>
> > +config.suffixes = ['.ll']<br>
><br>
> Since the tests should probably be moved to `test/Linker`, we probably<br>
> don't need this.<br>
><br>
> > diff --git a/tools/llvm-link/llvm-link.cpp b/tools/llvm-link/llvm-link.cpp<br>
> > index 6924aa5..54d49f5 100644<br>
> > --- a/tools/llvm-link/llvm-link.cpp<br>
> > +++ b/tools/llvm-link/llvm-link.cpp<br>
> > @@ -38,6 +38,11 @@ static cl::list<std::string><br>
> > InputFilenames(cl::Positional, cl::OneOrMore,<br>
> > cl::desc("<input bitcode files>"));<br>
> ><br>
> > +static cl::list<std::string><br>
> > +OverridingInputs("override", cl::ZeroOrMore, cl::value_desc("filename"),<br>
> > + cl::desc("input bitcode file which can "<br>
> > + "override previously defined symbol"));<br>
> > +<br>
> > static cl::opt<std::string><br>
> > OutputFilename("o", cl::desc("Override output filename"), cl::init("-"),<br>
> > cl::value_desc("filename"));<br>
> > @@ -109,24 +114,29 @@ int main(int argc, char **argv) {<br>
> > auto Composite = make_unique<Module>("llvm-link", Context);<br>
> > Linker L(Composite.get(), diagnosticHandler);<br>
> ><br>
> > - for (unsigned i = 0; i < InputFilenames.size(); ++i) {<br>
> > - std::unique_ptr<Module> M = loadFile(argv[0], InputFilenames[i], Context);<br>
> > - if (!M.get()) {<br>
> > - errs() << argv[0] << ": error loading file '" <<InputFilenames[i]<< "'\n";<br>
> > - return 1;<br>
> > - }<br>
> > + auto linkFiles = [&](const cl::list<std::string>& Files, bool Override) {<br>
> > + for (auto File : Files) {<br>
> > + std::unique_ptr<Module> M = loadFile(argv[0], File, Context);<br>
> > + if (!M.get()) {<br>
> > + errs() << argv[0] << ": error loading file '" << File << "'\n";<br>
> > + exit(1);<br>
> > + }<br>
> ><br>
> > - if (verifyModule(*M, &errs())) {<br>
> > - errs() << argv[0] << ": " << InputFilenames[i]<br>
> > - << ": error: input module is broken!\n";<br>
> > - return 1;<br>
> > - }<br>
> > + if (verifyModule(*M, &errs())) {<br>
> > + errs() << argv[0] << ": " << File<br>
> > + << ": error: input module is broken!\n";<br>
> > + exit(1);<br>
> > + }<br>
> ><br>
> > - if (Verbose) errs() << "Linking in '" << InputFilenames[i] << "'\n";<br>
> > + if (Verbose) errs() << "Linking in '" << File << "'\n";<br>
> ><br>
> > - if (L.linkInModule(M.get()))<br>
> > - return 1;<br>
> > - }<br>
> > + if (L.linkInModule(M.get(), Override))<br>
> > + exit(1);<br>
> > + }<br>
> > + };<br>
> > +<br>
> > + linkFiles(InputFilenames, false);<br>
><br>
> It'd be nice for the refactoring part of this (i.e., the part that<br>
> changes all the indentation) to be in a separate NFC (no functionality<br>
> change) commit so it's easier to see what has really changed.<br>
><br>
> Done.<br>
<br>
</div></div>Ah, but the order seems wrong.<br>
<br>
In this case, anyway, putting the NFC commit first gives you two<br>
distinct changes: (1) refactor to make the code reusable, (2) actually<br>
reuse the code, adding a flag and condition, etc. Each commit stands<br>
on its own and is easy to post-commit review (first commit, refactoring<br>
noise; second commit, small functional change).<br>
<br>
The order you chose (NFC second) adds the duplicate/different code and<br>
then cleans it up. The first patch -- the one with real changes -- is<br>
*bigger* than your original patch, whereas my goal was to minimize the<br>
noise around the functional changes. Please reverse the commits.<br></blockquote><div><br></div></div></div><div>Swapped the order of the commits.<br></div><span class=""><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> + auto linkFiles = [&](const cl::list<std::string>& Files, bool Override) {<br>
<br>
</span>Since I've gotten all nit-picky here anyway... I wonder whether using a<br>
lambda function is actually useful here. It saves passing `argv[0]` and<br>
`Context`, but I'm not sure that's buying much over just creating a<br>
normal `static` function. The latter seems just as clear, or maybe even<br>
clearer. I don't have a strong opinion; it's up to you.<br></blockquote><div><br></div></span><div>Switched to a static function since that's what the rest of the code seems to<br></div><div>be doing.<br> <br></div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Subject: [PATCH 1/2] [llvm-link] Add -override flag to prefer one of a<br>
> duplicate symbol.<br>
<br>
Since you've attached patches from git, I've had a look at the commit<br>
messages, and the functional one in particular doesn't really explain<br>
the motivation.<br>
<br>
When you actually commit this you should describe the workflow that<br>
you're planning to use this in, or otherwise explain why this flag is<br>
useful.<br>
<br>
(Do you have commit access? If not, let me know when you send the next<br>
version of the patch and I'll commit it for you. In that case I can<br>
just reuse my own writeup from earlier in the thread if you want.)<br></blockquote><div><br></div></span><div>No, I don't have commit access so that'd be great!<br> <br></div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> ---<br>
> include/llvm/Linker/Linker.h | 4 +++-<br>
> lib/Linker/LinkModules.cpp | 19 +++++++++++++++----<br>
> test/Linker/Inputs/manual-override.ll | 4 ++++<br>
> test/Linker/manual-override.ll | 19 +++++++++++++++++++<br>
> tools/llvm-link/llvm-link.cpp | 26 +++++++++++++++++++++++++-<br>
<br>
The testcases look a little light. Can you confirm what should happen<br>
if the linkage types differ? For example, if the -override is `linkonce`<br>
and the normal function is `weak`, IIUC the code will choose the<br>
`linkonce` function (thusly ignoring linkage rules). This makes sense to<br>
me -- that's what -override sounds like it should do -- but you should<br>
test it.<br></blockquote><div><br></div></span><div>Added.<br> <br></div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, a test confirming that functions with `internal` linkage are<br>
unaffected makes sense to me. I.e., if you have `internal foo` in the<br>
main file, and normal (external) linkage in the -override file, the<br>
`internal foo` should be renamed out of the way instead of replaced.<br>
Similarly, an `internal foo` from the -override file should get renamed<br>
on collisions. (It looks like this is what will happen already, and it<br>
seems right to me, but I think it deserves a test.)<br></blockquote><div><br></div></span><div>These too, but I did have to make one more small change to lib/Linker <br></div><div>to get the latter case working. Otherwise it would just ignore the internal <br></div><div>symbol from the -override input. <br> <br></div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Finally, one whitespace bug below:<br>
<br>
> diff --git a/tools/llvm-link/llvm-link.cpp b/tools/llvm-link/llvm-link.cpp<br>
> index 485a7b2..54d49f5 100644<br>
> --- a/tools/llvm-link/llvm-link.cpp<br>
> +++ b/tools/llvm-link/llvm-link.cpp<br>
> @@ -114,43 +114,29 @@ int main(int argc, char **argv) {<br>
<span>> auto Composite = make_unique<Module>("llvm-link", Context);<br>
> Linker L(Composite.get(), diagnosticHandler);<br>
><br>
> - for (unsigned i = 0; i < InputFilenames.size(); ++i) {<br>
> - std::unique_ptr<Module> M = loadFile(argv[0], InputFilenames[i], Context);<br>
> - if (!M.get()) {<br>
> - errs() << argv[0] << ": error loading file '" <<InputFilenames[i]<< "'\n";<br>
> - return 1;<br>
</span><span>> + auto linkFiles = [&](const cl::list<std::string>& Files, bool Override) {<br>
<br>
</span>`const cl::list<std::string>& Files` should be<br>
`const cl::list<std::string> &Files`. I recommend adding clang-format-diff<br>
to your workflow:<br>
<br>
<a href="http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting" target="_blank">http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting</a></blockquote><div><br></div></span><div>Ah cool, thanks!<br> <br></div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<span><br>
> + for (auto File : Files) {<br>
> + std::unique_ptr<Module> M = loadFile(argv[0], File, Context);<br>
> + if (!M.get()) {<br>
> + errs() << argv[0] << ": error loading file '" << File << "'\n";<br>
> + exit(1);<br>
> + }<br>
><br>
<br>
<br>
</span><span>> > + linkFiles(OverridingInputs, true);<br>
> ><br>
> > if (DumpAsm) errs() << "Here's the assembly:\n" << *Composite;<br>
> ><br>
> > --<br>
> > 2.3.0 (Apple Git-54)<br>
> ><br>
> ><br>
><br>
><br>
><br>
><br>
</span>> <0001-llvm-link-Add-override-flag-to-prefer-one-of-a-dupli.patch><0002-llvm-link-Factor-out-code-for-handling-regular-and-o.patch><br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div>