[PATCH] D34448: bug33527 - Linker::LinkOnlyNeeded should import AppendingLinkage globals

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 22 08:00:16 PDT 2017


tejohnson added inline comments.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:35
 #include "llvm/LTO/LTO.h"
-#include "llvm/Linker/Linker.h"
 #include "llvm/MC/SubtargetFeature.h"
----------------
belleyb wrote:
> tejohnson wrote:
> > belleyb wrote:
> > > Linker isn't used by ThinLTO. Include can be removed.
> > Can you submit this one as a separate NFC change (no patch review needed)? Thanks.
> I'm not sure how to proceed as I don't have commit access myself. :-(
Were planning to get commit access for the patch, or if you want I can commit this part for you now?


================
Comment at: unittests/Linker/LinkModulesTest.cpp:364
 
+TEST_F(LinkModuleTest, ImportIntrinsicGlobalVariables_ctors1) {
+  SMDiagnostic Err;
----------------
belleyb wrote:
> tejohnson wrote:
> > belleyb wrote:
> > > Starting from here are added unit tests demonstrating the actual issue and proving the suggested fix is correct.
> > > 
> > > I was wondering if it would have been more appropriate to write those using llvm-link and FileCheck instead. Let me know if you would prefer that alternate approach instead.
> > The unit test is quite long - I typically add tests using FileCheck for this type of issue, if you can demonstrate the issue.
> Sure, I'll try...
Looking at this again, I see it is not one long unit test but multiple tests. However, I still have a preference for a FileCheck lit test, it is just easier to read and see what is being tested. It looks like it should be reproducible using the inputs you have below and llvm-link, piping the output through FileCheck.


================
Comment at: unittests/Linker/LinkModulesTest.cpp:402
+  // Intrisic global variables must be imported since the
+  // ImportIntrinsicGlobalVariables flag has been specified.
+  auto const *ctor1 = Dst->getFunction("ctor1");
----------------
There are a number of references in the unit test to this flag, which looks like it was from an old approach you were considering, but not actually using here. Although I guess this comment is moot if you transition to using lit/FileCheck based tests...


https://reviews.llvm.org/D34448





More information about the llvm-commits mailing list