[PATCH] D18415: [LTO] Basic support for internalize.

[Rafael Ávila de Espíndola via llvm-commits]

rafael added a comment.

Please extend the test to coven a symbol that is *not* internalized.


================
Comment at: ELF/LTO.cpp:79
@@ -77,2 +78,3 @@
 
+  SmallPtrSet<GlobalValue *, 8> Used;
   for (const BasicSymbolRef &Sym : Obj->symbols()) {
----------------
This is now unused.

================
Comment at: ELF/SymbolTable.cpp:225
@@ +224,3 @@
+    if (auto *BC = dyn_cast<DefinedBitcode>(New))
+      BC->setUsedInRegularObj();
+
----------------
So, do you need to set this in here at all?

We already have

  if (IsUsedInRegularObj || Other->IsUsedInRegularObj)
    IsUsedInRegularObj = Other->IsUsedInRegularObj = true;

================
Comment at: test/ELF/lto/internalize-basic.ll:4
@@ +3,3 @@
+; RUN: ld.lld -m elf_x86_64 %t.o -o %t2 -save-temps
+; RUN: llvm-dis %t2.lto.bc -o - | FileCheck %s
+
----------------
silvas wrote:
> davide wrote:
> > grimar wrote:
> > > "-o -" ?  That looks a bit wierd to have "-" as a file name IMO, may be just standart temporary file would be better here ?
> > It's stdout.
> Doesn't llvm-dis default to stdout? I don't think `-o -` os needed.
It is not needed if you use it as

llvm-dis < %t2.lto.bc | FileCheck

Which is probably a bit better as llvm-dis doesn't get the filename.

================
Comment at: test/ELF/lto/internalize-basic.ll:17
@@ +16,3 @@
+
+; Check that foo function is correctly internalized.
+; CHECK: define internal void @foo()
----------------
What about _start?


http://reviews.llvm.org/D18415