Page MenuHomePhabricator

[NVPTX] Fix a segfault for bitcasted calls with byval params

Authored by ldrumm on Sep 23 2022, 9:25 AM.


`getFunctionParamOptimizedAlign` was being passed a null function
  argument when getting the callee of a bitcasted function symbol. This is
  because `CallBase::getCalledFunction` does not look through bitcasts.
  There is already code to handle this case in
  `NVPTXTargetLowering::getArgumentAlignment`, which is now hoisted into
  an NVPTX util.
  The alignment computation now gracefully handles computing alignment of
  virtual functions with a check for null.

Diff Detail

Unit TestsFailed

70 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::lljit-initialize-deinitialize.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/lli -jit-kind=orc -jit-linker=jitlink -orc-runtime=/var/lib/buildkite-agent/builds/llvm-project/build/./lib/clang/16.0.0/lib/x86_64-unknown-linux-gnu/liborc_rt.a /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/lljit-initialize-deinitialize.ll | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/lljit-initialize-deinitialize.ll
170 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::priority-static-initializer.S
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/priority-static-initializer.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/priority-static-initializer.S
140 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-cxa-atexit.S
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-cxa-atexit.S

Event Timeline

ldrumm created this revision.Sep 23 2022, 9:25 AM
ldrumm requested review of this revision.Sep 23 2022, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2022, 9:25 AM
tra accepted this revision.Sep 23 2022, 11:24 AM

LGTM with few nits.


Would we ever have an instruction other than Call which would also carry explicit alignment metadata?


Nit: if (!(F && F->hasLocalLinkage())) would be a bit easier to understand, IMO.

This revision is now accepted and ready to land.Sep 23 2022, 11:24 AM
ldrumm updated this revision to Diff 462877.Sep 26 2022, 5:58 AM
ldrumm marked an inline comment as done.

Code review suggestions

ldrumm marked an inline comment as done and an inline comment as not done.Sep 26 2022, 5:59 AM
ldrumm added inline comments.

allocas, loads, and stores, though I'm not sure if that's what you're asking. Could you expand a little on what you mean? The code here is only dealing with argument alignment so in isolation the answer is "no", but I have a feeling you might be hinting at something deeper ?


Yes. I actually prefer that style too. Too many competing style guides in my head!

tra added inline comments.Sep 26 2022, 11:09 AM

If CallBase can be an instruction other than Call, will we be able to get argument alignment via getAlign() and if so, should we?

Another observation here is that previously digging through bitcasts was done under if ( dyn_cast<CallInst>(CB)) while now it's done for any CB instruction. It looks like it should work (getMaybeBitcastedCallee will bail out on other instructions, if it can't get ), but was that done that intentionally? If looking through bitcasts is applicable to the Call instruction only, I'd prefer to keep it under the if to make it obvious.

nikic added a subscriber: nikic.Sep 26 2022, 11:47 AM
nikic added inline comments.

The whole function can be reduced to dyn_cast<Function>(CB->getCalledOperand()->stripPointerCasts()).

ldrumm updated this revision to Diff 466799.Oct 11 2022, 6:58 AM
ldrumm marked an inline comment as done.
ldrumm added inline comments.

I don't think this can ever be anything other than a call. It's within if (const auto *CI = dyn_cast<CallInst>(CB)) {. The getMaybeBitcastedCallee is indeed used in more possible places but I figured this to be safe in the current usage.


Thanks. TIL about stripPointerCasts

ldrumm closed this revision.Oct 18 2022, 6:44 AM

Merged in wherein I forgot to note the differential revision