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