Page MenuHomePhabricator

[IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction
ClosedPublic

Authored by madhur13490 on Mar 18 2021, 12:14 PM.

Details

Summary

This patch enhances hasAddressTaken() to ignore bitcasts as a
callee in callbase instruction. Such bitcast usage doesn't really take
the address in a useful meaningful way.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

The bot shows failure of bad-signature.c at https://reviews.llvm.org/B95560. When ran locally it does report the correct values and thus pass. I am not sure how to fix the test failure.
Failure in tile-and-distribute.mlir seems unrelated to this change.

@jdoerfert/ @arsenm / @rampitec , please advise.

The bots are imperfect, I would say it's unrelated

The bot shows failure of bad-signature.c at https://reviews.llvm.org/B95560. When ran locally it does report the correct values and thus pass. I am not sure how to fix the test failure.
Failure in tile-and-distribute.mlir seems unrelated to this change.

@jdoerfert/ @arsenm / @rampitec , please advise.

The bots are imperfect, I would say it's unrelated

Thanks. This patch now addresses all known failures and thus I don't expect it to be a moving target from now.

rampitec added inline comments.Mar 25 2021, 1:14 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

I do not see why a user of bitcast must be necessarily a call.

2184

Same here, it is not necessarily a call.

arsenm added inline comments.Mar 25 2021, 1:19 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

This could be a function in a constant initializer for a global

rampitec added inline comments.Mar 25 2021, 1:24 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

Right, that's what I mean. It could be anything, even a compare.

go back to dyn_cast to handle all users

madhur13490 added inline comments.Mar 25 2021, 9:41 PM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

Right, now I remember why I had dyn_cast and return in previous diff.

fhahn added a subscriber: fhahn.Mar 26 2021, 1:52 AM
fhahn added inline comments.
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

Is there a test where the user is not a bit cast ? If not, please add one, similar for the other places that were changed back to dyn_cast

rampitec added inline comments.Mar 26 2021, 9:39 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

You do not check that user is a callee.

I think this GlobalOpt patch is a separate change from the original one and needs to go first.

llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

You do not check that user is a callee.

And the original code does not check it either. This also warrants a test.

specifically check for callee

madhur13490 added inline comments.Mar 26 2021, 10:26 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

You do not check that user is a callee.

Done. I think we can introduce a utility function `CallBase* isCallBaseCallee(Use &U)` which can outline the below checks

User *UU = U.getUser();
 CallBase *CB = dyn_cast<CallBase>(UU);
 if (!CB)
   return nullptr;
 if (!CB->isCallee(&U))
   return nullptr;
 return CB

Not sure if this is an overkill.

This should work I guess. It needs tests for non-callee use and non call base uses. I would also prefer to GlobalOpt change to be extracted into a separate patch.

rampitec added inline comments.Mar 26 2021, 10:31 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

You now have it in 3 different places, so it may make sense. It would look better if you combine conditions like '!CB || !CB->isCallee(&U))' and also 'isSomething' function shall return bool, not a pointer.

This should work I guess. It needs tests for non-callee use and non call base uses. I would also prefer to GlobalOpt change to be extracted into a separate patch.

non call base users - What all such cases possible?We can use invoke but that is also CallBase.
non callee - ignore-bitcast-call-argument-callee.ll is similar to what to suggest.

This should work I guess. It needs tests for non-callee use and non call base uses. I would also prefer to GlobalOpt change to be extracted into a separate patch.

non call base users - What all such cases possible?We can use invoke but that is also CallBase.

That is more than possible. Take a pointer and store it, compare to another pointer, bitcast and store or compare.

non callee - ignore-bitcast-call-argument-callee.ll is similar to what to suggest.

Yes, but this is not for global opt.

This should work I guess. It needs tests for non-callee use and non call base uses. I would also prefer to GlobalOpt change to be extracted into a separate patch.

non call base users - What all such cases possible?We can use invoke but that is also CallBase.

That is more than possible. Take a pointer and store it, compare to another pointer, bitcast and store or compare.

Yes, but we should also consider about hasAddressTaken(). I am now starting to think if hasAddressTaken() needs to be more matured. Technically, you can cast a function pointer to global i8* and do all operations you suggest (e.g. use it as an operand in load) but hasAddressTaken() would return true in such cases because I am considering only CallBase users. If it needs to be more matured then we need to consider *all* instructions which take address as one of the operands.

non callee - ignore-bitcast-call-argument-callee.ll is similar to what to suggest.

Yes, but this is not for global opt.

This should work I guess. It needs tests for non-callee use and non call base uses. I would also prefer to GlobalOpt change to be extracted into a separate patch.

non call base users - What all such cases possible?We can use invoke but that is also CallBase.

That is more than possible. Take a pointer and store it, compare to another pointer, bitcast and store or compare.

Yes, but we should also consider about hasAddressTaken(). I am now starting to think if hasAddressTaken() needs to be more matured. Technically, you can cast a function pointer to global i8* and do all operations you suggest (e.g. use it as an operand in load) but hasAddressTaken() would return true in such cases because I am considering only CallBase users. If it needs to be more matured then we need to consider *all* instructions which take address as one of the operands.

Not necessarily. We ignore some uses in hasAddressTaken() and that can leak to this place.

add new test

madhur13490 added inline comments.Mar 27 2021, 6:32 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
2154

blockaddr-bitcast-fastcc.ll test address this.

llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll
2

This test case demonstates that fastcc is not set on function bar because its address taken value is true as hasAddressTaken() is used with its default param values.

llvm/test/Transforms/GlobalOpt/bitcast-call-argument-fastcc.ll
2

Test case when bitcast is a call argument.

llvm/test/Transforms/GlobalOpt/blockaddr-bitcast-fastcc.ll
2

This test case demonstrates that fastcc is now set on function "g" unlike trunk which skips this function. This test also covers non call bases users case.

foad added a subscriber: foad.Mar 29 2021, 8:30 AM
foad added inline comments.
llvm/lib/IR/Function.cpp
1643

This does not respect IgnoreCallbackUses, IgnoreAssumeLikeCalls, IgnoreLLVMUsed. Also it does not handle bitcasts-of-bitcasts.

Wouldn't the whole function be better implemented as a worklist algorithm now?

SmallVector<Use &> WorkList(use_begin(), use_end());
while (!WorkList.empty()) {
  U = WorkList.pop_back_val();
  if (isa<BitCast>(U))
    WorkList.push_back(U.use_begin(), U.use_end());
  else if (isa<CallBase>(U))
    ... all the existing logic that handles Ignore* flags ...
}

It looks fine to me with a style changes in couple places and positive checks in new tests.

llvm/lib/IR/Function.cpp
1643

In general yes, but this patch is growing. This would be a good followup change.
The pass calls hasAddressTaken() without arguments so that is not exerciseable here as is. For the same reason I withdraw my request to split the patch. Also note this may change if this is landed: D96179.

llvm/lib/Transforms/IPO/GlobalOpt.cpp
2137

A readbility change would be nice:

if (!CB && !CB->isCallee(&U))
  return
2167

Same here: if (!CB && !CB->isCallee(&U))

llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll
14

You need to check something positive. You can just check it is 'define internal void @bar() {'.

llvm/test/Transforms/GlobalOpt/bitcast-call-argument-fastcc.ll
10

Same here.

foad added inline comments.Mar 30 2021, 2:15 AM
llvm/lib/IR/Function.cpp
1643

I disagree. You're changing a core API Function::hasAddressTaken in a way that could change the behaviour for all existing callers, whether they like it or not. So I think this part probably should be a separate patch and the implementation should be as rigorous as possible.

madhur13490 added inline comments.Mar 30 2021, 3:30 AM
llvm/lib/IR/Function.cpp
1643

I disagree. You're changing a core API Function::hasAddressTaken in a way that could change the behaviour for all existing callers, whether they like it or not.

In general sense I do agree agree about the impact and that is why the first diff of this patch had an option to have the new code under an optional flag. Please have a look at the diff and the followup discussion on it.

So I think this part probably should be a separate patch and the implementation should be as rigorous as possible.

I don't understand this part. Worklist based algorithm is more readble and maintenable but that is another way of implementing what this patch does and is not going to do anything different than this does functionality wise. If there is some functionality missing in this patch, then that's a different story. But I don't see that being a reason to block further progress on this patch.

foad added inline comments.Mar 30 2021, 3:38 AM
llvm/lib/IR/Function.cpp
1643

If there is some functionality missing in this patch, then that's a different story.

The code you added to check callers-via-bitcasts does not respect the Ignore* flags, which could cause problems for existing users of this function. The worklist thing was just a suggestion for how to fix that (and also handle bitcasts-of-bitcasts-... in a natural way, though I'm not sure if they ever occur in practice).

foad added inline comments.Mar 30 2021, 7:05 AM
llvm/lib/IR/Function.cpp
1643

Sorry, I was confused. I see now that you're just adding yet more cases which don't count as "taking the address" of the function, which shouldn't cause any problems. I still think this function is getting too complicated and could do with refactoring.

Addressed Stas' comments

madhur13490 marked an inline comment as done.Mar 30 2021, 7:25 AM
madhur13490 added inline comments.
llvm/test/Transforms/GlobalOpt/assumelike-bitcast-fastcc.ll
14

Added check for foo as well.

rampitec accepted this revision.Mar 30 2021, 9:31 AM

LGTM with a nit in 2 tests. Please also wait for other responses.

llvm/test/Transforms/GlobalOpt/bitcast-call-argument-fastcc.ll
11

Make a positive check for foo.

22

Also need positive check for test.

madhur13490 marked an inline comment as done.Apr 3 2021, 8:13 PM

Ping!

jdoerfert resigned from this revision.Apr 5 2021, 10:21 AM

Seems reasonable now, haven't looked into the Attributor test changes but I expect unrelated side effects. I'm not blocking this anymore.

make positive checks

This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2021, 2:24 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

This change broke the native build on x32 (x86_64 with 32-bit pointers) for me:

[46/3990] Building C object projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o
/glaubitz/llvm-project/stage1.install/bin/clang -DVISIBILITY_HIDDEN -D_DEBUG -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/glaubitz/llvm-project/compiler-rt/lib/builtins -Iinclude -I/glaubitz/llvm-project/llvm/include -fPIC -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wno-unused-parameter -O3   -m64 -fno-lto -std=c11 -UNDEBUG -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o -c /glaubitz/llvm-project/compiler-rt/lib/builtins/cpu_model.c
clang: /glaubitz/llvm-project/llvm/include/llvm/Support/Casting.h:269: typename llvm::cast_retty<X, Y*>::ret_type llvm::cast(Y*) [with X = llvm::CallBase; Y = llvm::User; typename llvm::cast_retty<X, Y*>::ret_type = llvm::CallBase*]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: /glaubitz/llvm-project/stage1.install/bin/clang -DVISIBILITY_HIDDEN -D_DEBUG -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/glaubitz/llvm-project/compiler-rt/lib/builtins -Iinclude -I/glaubitz/llvm-project/llvm/include -fPIC -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wno-unused-parameter -O3 -m64 -fno-lto -std=c11 -UNDEBUG -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/cpu_model.c.o -c /glaubitz/llvm-project/compiler-rt/lib/builtins/cpu_model.c
1.      <eof> parser at end of file
2.      Optimizer
 #0 0x01b23df2 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/glaubitz/llvm-project/stage1.install/bin/clang+0x1b23df2)
 #1 0x01b21e10 llvm::sys::RunSignalHandlers() (/glaubitz/llvm-project/stage1.install/bin/clang+0x1b21e10)
 #2 0x01a8ea17 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0xfffffffff7f021e0 __restore_rt (/lib/x86_64-linux-gnux32/libpthread.so.0+0x131e0)
 #4 0xfffffffff7a2886d raise (/lib/x86_64-linux-gnux32/libc.so.6+0x3286d)
 #5 0xfffffffff7a12515 abort (/lib/x86_64-linux-gnux32/libc.so.6+0x1c515)
 #6 0xfffffffff7a123fe (/lib/x86_64-linux-gnux32/libc.so.6+0x1c3fe)
 #7 0xfffffffff7a211ff (/lib/x86_64-linux-gnux32/libc.so.6+0x2b1ff)
 #8 0x019f65c0 llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>, llvm::function_ref<llvm::AnalysisResultsForFn (llvm::Function&)>) (/glaubitz/llvm-project/stage1.install/bin/clang+0x19f65c0)
 #9 0x015d3b7d llvm::IPSCCPPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x15d3b7d)
#10 0x02ae78dd llvm::detail::PassModel<llvm::Module, llvm::IPSCCPPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x2ae78dd)
#11 0x0146f513 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x146f513)
#12 0x01de06a1 (anonymous namespace)::EmitAssemblyHelper::EmitAssemblyWithNewPassManager(clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (.constprop.0) BackendUtil.cpp:0:0
#13 0x01de3ae2 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (/glaubitz/llvm-project/stage1.install/bin/clang+0x1de3ae2)
#14 0x02a844dd clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x2a844dd)
#15 0x0368be90 clang::ParseAST(clang::Sema&, bool, bool) (/glaubitz/llvm-project/stage1.install/bin/clang+0x368be90)
#16 0x02424b3c clang::FrontendAction::Execute() (/glaubitz/llvm-project/stage1.install/bin/clang+0x2424b3c)
#17 0x023bb424 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x23bb424)
#18 0x024e0fd3 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/glaubitz/llvm-project/stage1.install/bin/clang+0x24e0fd3)
#19 0x008dd673 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/glaubitz/llvm-project/stage1.install/bin/clang+0x8dd673)
#20 0x008d921d ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) driver.cpp:0:0
#21 0x02262c99 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const::'lambda'()>(int) Job.cpp:0:0
#22 0x01a8eb8d llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/glaubitz/llvm-project/stage1.install/bin/clang+0x1a8eb8d)
#23 0x02263ecb clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const (.part.0) Job.cpp:0:0
#24 0x0223911c clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const (/glaubitz/llvm-project/stage1.install/bin/clang+0x223911c)
#25 0x02239b37 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) const (/glaubitz/llvm-project/stage1.install/bin/clang+0x2239b37)
#26 0x022452f4 clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) (/glaubitz/llvm-project/stage1.install/bin/clang+0x22452f4)
#27 0x0085ecea main (/glaubitz/llvm-project/stage1.install/bin/clang+0x85ecea)
#28 0xfffffffff7a13da2 __libc_start_main (/lib/x86_64-linux-gnux32/libc.so.6+0x1dda2)
#29 0x008d8bbb _start (/glaubitz/llvm-project/stage1.install/bin/clang+0x8d8bbb)
clang-13: error: clang frontend command failed with exit code 134 (use -v to see invocation)
clang version 13.0.0
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /glaubitz/llvm-project/stage1.install/bin
clang-13: note: diagnostic msg:
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang-13: note: diagnostic msg: /tmp/cpu_model-3ac5e8.c
clang-13: note: diagnostic msg: /tmp/cpu_model-3ac5e8.sh
clang-13: note: diagnostic msg:

********************

I'll file a bug report.

@glaubitz Now that there's a repro - please could you revert this so @madhur13490 can investigate it offline?

@glaubitz Now that there's a repro - please could you revert this so @madhur13490 can investigate it offline?

I don't have commit access, unfortunately.

I have an auto-bisecting cron job that has identified this change as a second-stage regression on Fedora 34 (x86-64). Basically llvm-tblgen crashes reliably:

FAILED: lib/Target/X86/X86GenDAGISel.inc
cd /tmp/_update_lc/t && /tmp/_update_lc/t/bin/llvm-tblgen -gen-dag-isel -I /home/dave/ro_s/lp/llvm/lib/Target/X86 -I/tmp/_update_lc/t/include -I/home/dave/ro_s/lp/llvm/include -I /home/dave/ro_s/lp/llvm/lib/Target -omit-comments /home/dave/ro_s/lp/llvm/lib/Target/X86/X86.td --write-if-changed -o lib/Target/X86/X86GenDAGISel.inc -d lib/Target/X86/X86GenDAGISel.inc.d
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.  Program arguments: /tmp/_update_lc/t/bin/llvm-tblgen -gen-dag-isel -I /home/dave/ro_s/lp/llvm/lib/Target/X86 -I/tmp/_update_lc/t/include -I/home/dave/ro_s/lp/llvm/include -I /home/dave/ro_s/lp/llvm/lib/Target -omit-comments /home/dave/ro_s/lp/llvm/lib/Target/X86/X86.td --write-if-changed -o lib/Target/X86/X86GenDAGISel.inc -d lib/Target/X86/X86GenDAGISel.inc.d
#0 0x000000000050b153 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/tmp/_update_lc/t/bin/llvm-tblgen+0x50b153)
#1 0x00000000005094b2 llvm::sys::RunSignalHandlers() (/tmp/_update_lc/t/bin/llvm-tblgen+0x5094b2)
#2 0x000000000050b4fa SignalHandler(int) Signals.cpp:0:0
#3 0x00007ffff7fa6a00 __restore_rt sigaction.c:0:0
#4 0x0000000000316a7d (/tmp/_update_lc/t/bin/llvm-tblgen+0x316a7d)

Is this issue known?

We've also seen failures on all of our 2 stage bots e.g. http://lab.llvm.org:8011/#/builders/7/builds/2348. Which are fixed now this is reverted.

[494/5842] Building Attributes.inc...
FAILED: include/llvm/IR/Attributes.inc 
cd /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2 && /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2/bin/llvm-tblgen -gen-attrs -I /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include/llvm/IR -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2/include -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include/llvm/IR/Attributes.td --write-if-changed -o include/llvm/IR/Attributes.inc -d include/llvm/IR/Attributes.inc.d
pure virtual method called
terminate called without an active exception

Bisecting this I have seen clang-tblgen crash with a few different asserts. Bad alloc, one in erase() in StringMap, illegal instruction. Our bot doesn't have sanitizers on so it could be the same finicky memory corruption.

This PPC one does though https://lab.llvm.org/buildbot/#/builders/18/builds/1140 and saw similar failures. No immediate explanation but some configs worth checking locally before the reland.

foad added a comment.Apr 6 2021, 8:18 AM

I think it was also causing this failure in my Release+Asserts build:

/home/jayfoad2/git/llvm-project/llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll:150:16: error: CHECK-LABEL: expected string not found in input
; CHECK-LABEL: define internal void @branch_funnel(i8*
               ^
<stdin>:81:16: note: scanning from here
define i32 @fn3(i8* nocapture readonly %obj) local_unnamed_addr #2 {
               ^
<stdin>:95:18: note: possible intended match here
define hidden void @__typeid_typeid1_0_branch_funnel(i8* nest %0, ...) local_unnamed_addr #6 {
                 ^
...
Failed Tests (1):
  LLVM :: Transforms/WholeProgramDevirt/branch-funnel.ll

These failures were not caught by LLVM bots which run on a patch. Can I do a trial run of the bots and run all tests that would run when the patch lands?

These failures were not caught by LLVM bots which run on a patch. Can I do a trial run of the bots and run all tests that would run when the patch lands?

I don’t think LLVM is set up for that. Can you try two stage testing first?

madhur13490 added a comment.EditedApr 8 2021, 2:09 AM

We've also seen failures on all of our 2 stage bots e.g. http://lab.llvm.org:8011/#/builders/7/builds/2348. Which are fixed now this is reverted.

[494/5842] Building Attributes.inc...
FAILED: include/llvm/IR/Attributes.inc 
cd /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2 && /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2/bin/llvm-tblgen -gen-attrs -I /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include/llvm/IR -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-full/stage2/include -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include /home/tcwg-buildslave/worker/clang-cmake-aarch64-full/llvm/llvm/include/llvm/IR/Attributes.td --write-if-changed -o include/llvm/IR/Attributes.inc -d include/llvm/IR/Attributes.inc.d
pure virtual method called
terminate called without an active exception

Bisecting this I have seen clang-tblgen crash with a few different asserts. Bad alloc, one in erase() in StringMap, illegal instruction. Our bot doesn't have sanitizers on so it could be the same finicky memory corruption.

This PPC one does though https://lab.llvm.org/buildbot/#/builders/18/builds/1140 and saw similar failures. No immediate explanation but some configs worth checking locally before the reland.

Apparently my change exposes the memory corruption bug on X86 too. As the bots show, I can't generate .inc for almost all ARM .td files. Not sure how to fix this reliably. Surprisingly, the builds pass in Debug build type.

This change has wider impact than was expected as it affects several components and backends. Bootstrap builds fail with this change and as the buildbots show, this change generates a buggy Tablegen binary in release mode.

Debugging:

  1. Disabling the newly added code in hasAddressTaken(), we get sane Tablegen and builds pass. To note, debug builds pass with the change. This clearly shows that codegen is functionally different in release and debug mode for Tablegen.
  2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
  3. Running valgrind also did not reveal any new things.
  4. Doing ASAN build of the compiler led to several errors and drawing any conclusion from it is close to impossible for this patch.

However, if I disable IPSCCP pass on top of this patch then everything turns green. I tried to fix trivial issues in the pass but it seems the pass needs deeper and major fixes to make this patch land.

Till we fix issues in IPSCCP, this patch stands reverted. The whole intention of this patch was to handle a subset of cases for AMDGPU backend, specifically, needed for https://reviews.llvm.org/D99347.

arsenm added a comment.Apr 9 2021, 1:11 PM

This change has wider impact than was expected as it affects several components and backends. Bootstrap builds fail with this change and as the buildbots show, this change generates a buggy Tablegen binary in release mode.

Debugging:

  1. Disabling the newly added code in hasAddressTaken(), we get sane Tablegen and builds pass. To note, debug builds pass with the change. This clearly shows that codegen is functionally different in release and debug mode for Tablegen.
  2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
  3. Running valgrind also did not reveal any new things.
  4. Doing ASAN build of the compiler led to several errors and drawing any conclusion from it is close to impossible for this patch.

However, if I disable IPSCCP pass on top of this patch then everything turns green. I tried to fix trivial issues in the pass but it seems the pass needs deeper and major fixes to make this patch land.

Till we fix issues in IPSCCP, this patch stands reverted. The whole intention of this patch was to handle a subset of cases for AMDGPU backend, specifically, needed for https://reviews.llvm.org/D99347.

This doesn't necessarily mean there's a problem with IPSCCP. What is the failing IR?

This change has wider impact than was expected as it affects several components and backends. Bootstrap builds fail with this change and as the buildbots show, this change generates a buggy Tablegen binary in release mode.

Debugging:

  1. Disabling the newly added code in hasAddressTaken(), we get sane Tablegen and builds pass. To note, debug builds pass with the change. This clearly shows that codegen is functionally different in release and debug mode for Tablegen.
  2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
  3. Running valgrind also did not reveal any new things.
  4. Doing ASAN build of the compiler led to several errors and drawing any conclusion from it is close to impossible for this patch.

However, if I disable IPSCCP pass on top of this patch then everything turns green. I tried to fix trivial issues in the pass but it seems the pass needs deeper and major fixes to make this patch land.

Till we fix issues in IPSCCP, this patch stands reverted. The whole intention of this patch was to handle a subset of cases for AMDGPU backend, specifically, needed for https://reviews.llvm.org/D99347.

This doesn't necessarily mean there's a problem with IPSCCP. What is the failing IR?

There is no failing IR but the failing binary. Tablegen constitutes around ~200 C/C++ files and all I know is that the binary is misompiling. It is pretty difficult to say at this point if one single file is miscompiling or a bunch of them. All I know at this point is

  1. A lot of new functions are now added to IPSCCP's radar as hasAddressTaken() for them is 'false' now as it meets the condition my patch adds i.e all users of bitcast operator are CB. See llvm::canTrackArgumentsInterprocedurally(Function *F) which is called from IPSCCP.
  2. IPSCCP may not be the culprit but it is definitely a starting point of my investigation. There are obvious places in it which needs fix e.g. properly removing argmemonly and inaccessiblemem_or_argmemonly attributes here.

Are we sure this is not a single bad cast that evolves into something different if assertions are disabled. Is the cast problem located, that should be easy after all given the minimal reproducer in PR49861?

Are we sure this is not a single bad cast that evolves into something different if assertions are disabled. Is the cast problem located, that should be easy after all given the minimal reproducer in PR49861?

The two line reproducer points to one part of the problem and it does fix cpu_model.c assert (mentioned in PR49861) but it does not help the miscompilation of TableGen. All my experiments are done after I fix the problem pointed by assert in cpu_model.c.

  1. A lot of new functions are now added to IPSCCP's radar as hasAddressTaken() for them is 'false' now as it meets the condition my patch adds i.e all users of bitcast operator are CB. See llvm::canTrackArgumentsInterprocedurally(Function *F) which is called from IPSCCP.

You can use a Debug Counter and bisect it to the first function for which the hasAddressTaken change will cause the issue.
With the pre- and post-IPSCCP module we should be able to figure this out.