Page MenuHomePhabricator

__builtin_object_size problem with pointer as argument
ClosedPublic

Authored by spetrovic on Feb 17 2016, 9:02 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

spetrovic updated this revision to Diff 48197.Feb 17 2016, 9:02 AM
spetrovic retitled this revision from to __builtin_object_size problem with pointer as argument.
spetrovic updated this object.
spetrovic set the repository for this revision to rL LLVM.
spetrovic added a subscriber: llvm-commits.
ab added a subscriber: ab.Feb 17 2016, 11:30 AM

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.

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 updated this revision to Diff 48900.Feb 24 2016, 2:51 AM
spetrovic edited edge metadata.

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.

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

lib/Transforms/InstCombine/InstructionCombining.cpp
1944

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.

1980

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.

1980

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

spetrovic updated this revision to Diff 49033.Feb 25 2016, 4:27 AM
spetrovic marked 2 inline comments as done.
spetrovic added inline comments.
lib/Transforms/InstCombine/InstructionCombining.cpp
1980

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 edited edge metadata.

This LGTM. Thanks for the patch!

lib/Transforms/InstCombine/InstructionCombining.cpp
1980

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. :)

This revision is now accepted and ready to land.Feb 25 2016, 10:23 AM
This revision was automatically updated to reflect the committed changes.
spetrovic updated this revision to Diff 49977.Mar 7 2016, 10:08 AM
spetrovic edited edge metadata.

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 ?

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 edited edge metadata.Mar 7 2016, 5:38 PM

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)

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? :)

rankov added a subscriber: rankov.Mar 8 2016, 9:02 AM
spetrovic updated this revision to Diff 50048.Mar 8 2016, 9:25 AM
spetrovic edited edge metadata.

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

LGTM with one comment. Thanks!

lib/Transforms/InstCombine/InstructionCombining.cpp
1945–1969

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

spetrovic updated this revision to Diff 50126.Mar 9 2016, 5:59 AM
spetrovic marked an inline comment as done.

Comments addressed. Thanks!

petarj added a comment.Mar 9 2016, 6:18 AM

Relanded in r263011.