[PATCH] Don't localize globals that may serve as leak checker roots.

[Duncan Sands]

Hi Alexey,

On 26/09/13 11:16, Alexey Samsonov wrote:
>
> On Thu, Sep 26, 2013 at 1:02 PM, Duncan Sands <duncan.sands at gmail.com
> <mailto:duncan.sands at gmail.com>> wrote:
>
>     Hi Alexey,
>
>
>     On 25/09/13 13:32, Alexey Samsonov wrote:
>
>         Hi nicholas,
>
>         This change prevents GlobalOpt from turning globals into
>         stack-allocated locals, if these variables may serve as a leak checker
>         root. For instance, transforming
>             Foo *x;
>             int main() { x = new Foo(); }
>         into
>             int main() { Foo *x = new Foo(); }
>         will make LeakSanitizer treat "x" as definitely leaked, which is
>         undesirable.
>
>
>     having magic semantics for globals with a certain type seems like a bad idea
>     to me.  Here the global in effect has a secret external use, but that isn't
>     clear from the IR.  In my opinion it's only asking for trouble and it would be
>     better to make it explicit by requiring all GC roots to be used by (eg) a new
>     llvm.gc_root global, analogous to llvm.used.
>
>
> Just to clarify this - who should create llvm.gc_root array - do you suggest to
> add a new __attribute__
> the user should specify, or to emit llvm.gc_root in frontend,

this one: the front-end would stick globals in the array, for example using the
same logic as in isLeakCheckerRoot (but of course it could use different logic
if it likes).

Ciao, Duncan.

  or in the backend
> before running
> optimization passes?


>
>
>     Ciao, Duncan.
>
>
>         http://llvm-reviews.chandlerc.__com/D1754
>         <http://llvm-reviews.chandlerc.com/D1754>
>
>         Files:
>             test/Transforms/GlobalOpt/__metadata.ll
>             test/Transforms/GlobalOpt/__dont-localize-pointer.ll
>             lib/Transforms/IPO/GlobalOpt.__cpp
>
>         Index: test/Transforms/GlobalOpt/__metadata.ll
>         ==============================__==============================__=======
>         --- test/Transforms/GlobalOpt/__metadata.ll
>         +++ test/Transforms/GlobalOpt/__metadata.ll
>         @@ -3,24 +3,24 @@
>            ; PR6112 - When globalopt does RAUW(@G, %G), the metadata reference
>         should drop
>            ; to null.  Function local metadata that references @G from a
>         different function
>            ; to that containing %G should likewise drop to null.
>         - at G = internal global i8** null
>         + at G = internal global i32 0
>
>            define i32 @main(i32 %argc, i8** %argv) {
>            ; CHECK-LABEL: @main(
>            ; CHECK: %G = alloca
>         -  store i8** %argv, i8*** @G
>         +  store i32 %argc, i32* @G
>              ret i32 0
>            }
>
>            define void @foo(i32 %x) {
>         -  call void @llvm.foo(metadata !{i8*** @G, i32 %x})
>         +  call void @llvm.foo(metadata !{i32* @G, i32 %x})
>            ; CHECK: call void @llvm.foo(metadata !{null, i32 %x})
>              ret void
>            }
>
>            declare void @llvm.foo(metadata) nounwind readnone
>
>            !named = !{!0}
>
>         -!0 = metadata !{i8*** @G}
>         +!0 = metadata !{i32* @G}
>            ; CHECK: !0 = metadata !{null}
>         Index: test/Transforms/GlobalOpt/__dont-localize-pointer.ll
>         ==============================__==============================__=======
>         --- /dev/null
>         +++ test/Transforms/GlobalOpt/__dont-localize-pointer.ll
>         @@ -0,0 +1,10 @@
>         +; RUN: opt -S -globalopt < %s | FileCheck %s
>         +
>         + at G = internal global i8** null
>         +
>         +define i32 @main(i32 %argc, i8** %argv) {
>         +; CHECK-LABEL: @main(
>         +; CHECK-NOT: alloca
>         +  store i8** %argv, i8*** @G
>         +  ret i32 0
>         +}
>         Index: lib/Transforms/IPO/GlobalOpt.__cpp
>         ==============================__==============================__=======
>         --- lib/Transforms/IPO/GlobalOpt.__cpp
>         +++ lib/Transforms/IPO/GlobalOpt.__cpp
>         @@ -1951,7 +1951,8 @@
>                  GV->getType()->getElementType(__)->isSingleValueType() &&
>                  GS.AccessingFunction->getName(__) == "main" &&
>                  GS.AccessingFunction->__hasExternalLinkage() &&
>         -      GV->getType()->__getAddressSpace() == 0) {
>         +      GV->getType()->__getAddressSpace() == 0 &&
>         +      !isLeakCheckerRoot(GV)) {
>                DEBUG(dbgs() << "LOCALIZING GLOBAL: " << *GV);
>                Instruction &FirstI =
>         const_cast<Instruction&>(*GS.__AccessingFunction
>
>           ->getEntryBlock().begin());
>
>
>
>         _________________________________________________
>         llvm-commits mailing list
>         llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>         http://lists.cs.uiuc.edu/__mailman/listinfo/llvm-commits
>         <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>
>
>     _________________________________________________
>     llvm-commits mailing list
>     llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>     http://lists.cs.uiuc.edu/__mailman/listinfo/llvm-commits
>     <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>
>
>
>
> --
> Alexey Samsonov, MSK