__builtin_object_size problem with pointer as argument

[spetrovic]

Summary

This patch fixes calculating correct value for builtin_object_size function when pointer is used only in builtin_object_size function call and never after that.

[ab]

While I don't think the builtin is intended to be used this way (that is, this behavior is intentional), the only argument I have against this patch is that it's basically wasting time on "dead" code.

That said, why not try to evaluate the objectsize call when we're about to remove it?
In the testcase, I think that we erase the objectsize call (and the alloca) before evaluating it only because InstCombine examines the alloca first.

[george.burgess.iv]

Thanks for the patch!

That said, why not try to evaluate the objectsize call when we're about to remove it?

Agreed. If the @llvm.objectsize call points directly to the alloca we're trying to kill, then I think it should be trivial to evaluate on the spot. This way, we can go ahead and eliminate the alloca instead of hoping that InstCombine runs again to get rid of it for us. :)

[spetrovic]

Comments addressed.
Calculating of __builtin_object_size must be resolved before data that is required for calculating is deleted, so it's moved to be resolved first.

[george.burgess.iv]

Looks good overall -- just a few small comments for you.

lib/Transforms/InstCombine/InstructionCombining.cpp

1944–1963

Can we have a small comment here explaining why we iterate through Users twice? Something like "We must lower all @llvm.objectsize calls first, because they may use a bitcast/GEP of the alloca we're removing" might be helpful.

1976

Nit: Can we move this somewhere above replaceInstUsesWith? Having the RAUW update Users[i] seems pointless if we're just going to null it out anyway.

1976

Nit: The comment should say "Skip examining" :)

[spetrovic]

lib/Transforms/InstCombine/InstructionCombining.cpp

1976

I think there is no difference if we move this ( Users[i] = nullptr; ). This is a local variable, and I don't think that it can affect replaceInstUsesWith().

[george.burgess.iv]

This LGTM. Thanks for the patch!

lib/Transforms/InstCombine/InstructionCombining.cpp

1976

Users is a vector of WeakVH, which is a class that gets updated automatically when a RAUW happens. If you want to verify this, try adding assert(isa<IntrinsicInst>(Users[i])); just above replaceInstUsesWith, and assert(isa<ConstantInt>(Users[i])); just below it. Neither assert fires on my build.

Either way, it's just a nit, so I'll leave the decision to move it up to you. :)

[spetrovic]

We have problem when LLVM is built with UBSAN sanitizer with our solution. Problem was with Users[i] = nullptr; line, so this is new solution, now works properly. Do you have some comments ?

[george.burgess.iv]

That's... concerning, especially given that there doesn't seem to be a bug filed about this behavior. Can you paste the errors that UBSan gives you, please?

lib/Transforms/InstCombine/InstructionCombining.cpp

1962

This smells quadratic. Can we just partition Users so that the calls to objectsize are all at the front instead? Doing so would also let us either make this loop quite a bit smaller, or let us go back to having only one loop.

[petarj]

In D17337#369422, @george.burgess.iv wrote:

That's... concerning, especially given that there doesn't seem to be a bug filed about this behavior. Can you paste the errors that UBSan gives you, please?

I reverted the change since it broke LLVM UBSan bilder.

Here is the error:

FAIL: LLVM :: Transforms/InstCombine/builtin-object-size-ptr.ll (12303 of 15979)
******************** TEST 'LLVM :: Transforms/InstCombine/builtin-object-size-ptr.ll' FAILED ********************
Script:
--
/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/./bin/opt -instcombine -S < /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/test/Transforms/InstCombine/builtin-object-size-ptr.ll | /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/./bin/FileCheck /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/test/Transforms/InstCombine/builtin-object-size-ptr.ll
--
Exit Code: 2

Command Output (stderr):
--
/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/include/llvm/IR/ValueHandle.h:101:37: runtime error: reference binding to null pointer of type 'llvm::Value'
    #0 0x1aa9282 in llvm::ValueHandleBase::operator*() const /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/include/llvm/IR/ValueHandle.h:101:30
    #1 0x22b1149 in llvm::InstCombiner::visitAllocSite(llvm::Instruction&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1964:51
    #2 0x2341631 in llvm::InstCombiner::visitAllocaInst(llvm::AllocaInst&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:308:10
    #3 0x22b5686 in llvm::InstCombiner::run() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2839:31
    #4 0x22b6856 in combineInstructionsOverFunction(llvm::Function&, llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::DominatorTree&, llvm::LoopInfo*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3065:16
    #5 0x22b7e34 in (anonymous namespace)::InstructionCombiningPass::runOnFunction(llvm::Function&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3140:10
    #6 0x20c952e in llvm::FPPassManager::runOnFunction(llvm::Function&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/IR/LegacyPassManager.cpp:1550:23
    #7 0x20c990b in llvm::FPPassManager::runOnModule(llvm::Module&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/IR/LegacyPassManager.cpp:1571:16
    #8 0x20ca420 in (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/IR/LegacyPassManager.cpp:1627:23
    #9 0x20c9d55 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/lib/IR/LegacyPassManager.cpp:1730:16
    #10 0x947df5 in main /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/opt/opt.cpp:628:3
    #11 0x7f3096470ec4 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
    #12 0x8ebc36 in _start (/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/bin/opt+0x8ebc36)

[george.burgess.iv]

Ahh, I see. It looks like the issue was when Users[i] is null in

Instruction *I = cast_or_null<Instruction>(&*Users[i]);
if (!I) continue;

In that case, it seems that we could fix the issue by moving the null check above the cast. Can we try that instead? :)

[spetrovic]

Moved null check above cast, and changed cast_or_null to cast.

[george.burgess.iv]

LGTM with one comment. Thanks!

lib/Transforms/InstCombine/InstructionCombining.cpp

1965

Please make the same change here, so the code is consistent.

[spetrovic]

Comments addressed. Thanks!

[petarj]

Relanded in r263011.

lib/Transforms/InstCombine/InstructionCombining.cpp

 }
 
 Instruction *InstCombiner::visitAllocSite(Instruction &MI) {
   // If we have a malloc call which is only used in any amount of comparisons
   // to null and free calls, delete the calls and replace the comparisons with
   // true or false as appropriate.
   SmallVector<WeakVH, 64> Users;
   if (isAllocSiteRemovable(&MI, Users, TLI)) {
     for (unsigned i = 0, e = Users.size(); i != e; ++i) {
+      // Lowering all @llvm.objectsize calls first because they may
+      // use a bitcast/GEP of the alloca we are removing.
+      Instruction *I = cast_or_null<Instruction>(&*Users[i]);
+      if (!I) continue;
+
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
+        if (II->getIntrinsicID() == Intrinsic::objectsize) {
+          uint64_t Size;
+          if (!getObjectSize(II->getArgOperand(0), Size, DL, TLI)) {
+            ConstantInt *CI = cast<ConstantInt>(II->getArgOperand(1));
+            Size = CI->isZero() ? -1ULL : 0;
+          }
+          replaceInstUsesWith(*I, ConstantInt::get(I->getType(), Size));
+          eraseInstFromFunction(*I);
+          Users[i] = nullptr; // Skip examining in next loop.
+        }
+      }
+    }
+    for (unsigned i = 0, e = Users.size(); i != e; ++i) {
       Instruction *I = cast_or_null<Instruction>(&*Users[i]);
       if (!I) continue;
 
       if (ICmpInst *C = dyn_cast<ICmpInst>(I)) {
         replaceInstUsesWith(*C,
                             ConstantInt::get(Type::getInt1Ty(C->getContext()),
                                              C->isFalseWhenEqual()));
       } else if (isa<BitCastInst>(I) || isa<GetElementPtrInst>(I)) {
         replaceInstUsesWith(*I, UndefValue::get(I->getType()));
-      } else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
-        if (II->getIntrinsicID() == Intrinsic::objectsize) {
-          ConstantInt *CI = cast<ConstantInt>(II->getArgOperand(1));
-          uint64_t DontKnow = CI->isZero() ? -1ULL : 0;
-          replaceInstUsesWith(*I, ConstantInt::get(I->getType(), DontKnow));
-        }
       }
       eraseInstFromFunction(*I);
     }
 
     if (InvokeInst *II = dyn_cast<InvokeInst>(&MI)) {
       // Replace invoke with a NOP intrinsic to maintain the original CFG
       Module *M = II->getModule();
       Function *F = Intrinsic::getDeclaration(M, Intrinsic::donothing);
       InvokeInst::Create(F, II->getNormalDest(), II->getUnwindDest(),
                          None, "", II->getParent());
     }
     return eraseInstFromFunction(MI);

test/Transforms/InstCombine/builtin-object-size-ptr.ll

+; RUN: opt -instcombine -S < %s | FileCheck %s
+
+; int foo() {
+; struct V { char buf1[10];
+;            int b;
+;            char buf2[10];
+;           } var;
+;
+;           char *p = &var.buf1[1];
+;           return __builtin_object_size (p, 0);
+; }
+
+%struct.V = type { [10 x i8], i32, [10 x i8] }
+
+define i32 @foo() #0 {
+entry:
+  %var = alloca %struct.V, align 4
+  %0 = bitcast %struct.V* %var to i8*
+  call void @llvm.lifetime.start(i64 28, i8* %0) #3
+  %buf1 = getelementptr inbounds %struct.V, %struct.V* %var, i32 0, i32 0
+  %arrayidx = getelementptr inbounds [10 x i8], [10 x i8]* %buf1, i64 0, i64 1
+  %1 = call i64 @llvm.objectsize.i64.p0i8(i8* %arrayidx, i1 false)
+  %conv = trunc i64 %1 to i32
+  call void @llvm.lifetime.end(i64 28, i8* %0) #3
+  ret i32 %conv
+; CHECK: ret i32 27
+; CHECK-NOT: ret i32 -1
+}
+
+declare void @llvm.lifetime.start(i64, i8* nocapture) #1
+
+declare i64 @llvm.objectsize.i64.p0i8(i8*, i1) #2
+
+declare void @llvm.lifetime.end(i64, i8* nocapture) #1