Page MenuHomePhabricator

[CodeGen] Don't create a fake FunctionDecl when generating block/block_byref copy/dispose helper functions
ClosedPublic

Authored by ahatanak on Jun 10 2021, 5:53 PM.

Details

Summary

These fake functions were causing clang to crash when https://reviews.llvm.org/D98799 made some changes.

The line number information is no longer emitted for the __Block_byref_object helper functions (see clang/test/CodeGenCXX/debug-info-blocks.cpp), but I think that's okay since the line numbers were incorrect when the helper functions were shared among multiple __block variables. For example, the line number for __Block_byref_object_copy_ and __Block_byref_object_dispose_ was the line number for __block id b in func0, which was incorrect if the helper functions were called when func1 was called.

void (^gb)();

int func0() {
  __block id b;
  gb = ^{ (void)b; };
  return 0;
}

int func1() {
  __block id b;
  gb = ^{ (void)b; };
  return 0;
}

Diff Detail

Event Timeline

ahatanak requested review of this revision.Jun 10 2021, 5:53 PM
ahatanak created this revision.

Also, is it okay to emit the linkageName, but not the name for the helper functions? It seems okay to me, but I'm not sure.

Also, is it okay to emit the linkageName, but not the name for the helper functions? It seems okay to me, but I'm not sure.

Not 100% sure - perhaps @aprantl can weigh in on this. If you can find some precedent/existing cases where we do that, might help provide some confidence (one way I'd do that would be to put an assertion in the CGDebugInfo code that makes the choice about which names to add - asserting if only a linkage name is added and no non-linkage name - then run check-clang and see what, if any, tests fail - that'll identify cases that have this property)

C++ non-virtual thunks and global initialization functions (_GLOBAL__sub_I_*) have only linkage names.

dblaikie accepted this revision.Jun 15 2021, 11:18 AM

C++ non-virtual thunks and global initialization functions (_GLOBAL__sub_I_*) have only linkage names.

Works for me - and the debug info codegen looks like it does handle this situation intentionally: https://github.com/llvm/llvm-project/blob/a11880468e556d20ad9b0d43a1ff43daf62d49b2/clang/lib/CodeGen/CGDebugInfo.cpp#L3896.

Maybe give it a few days (or directly ask) to see if @aprantl has further thoughts, since I'm not especially familiar with Objective C things, but it looks pretty right/good enough for me.

This revision is now accepted and ready to land.Jun 15 2021, 11:18 AM
aprantl accepted this revision.Jun 22 2021, 9:07 AM

Block copy helpers don't have any source code associated with them, so as long as they are described by a DISubprogram, the exact format of it most likely does not matter.

clang/lib/CodeGen/CGBlocks.cpp
1956–1957

This comment refers to the FD->setImplicit(); that was removed.

2142–2143

Same here — the comment should be removed.

ahatanak updated this revision to Diff 353729.Jun 22 2021, 11:33 AM
ahatanak marked 2 inline comments as done.

Delete comments.

This revision was landed with ongoing or failed builds.Jun 22 2021, 11:43 AM
This revision was automatically updated to reflect the committed changes.
zequanwu added a subscriber: zequanwu.EditedJun 22 2021, 9:51 PM

Hi, this caused compiler crash with error "Assertion `cast<DISubprogram>(Scope)->describes(&MF->getFunction())' failed." on iOS platforms. So, I reverted it.
I'm working on a reduced repro.

fhahn added a subscriber: fhahn.Jun 23 2021, 2:52 AM

Hi, this caused compiler crash with error "Assertion `cast<DISubprogram>(Scope)->describes(&MF->getFunction())' failed." on iOS platforms. So, I reverted it.
I'm working on a reduced repro.

FYI this also caused a failure on GreenDragon, with -verify-machineinstrs: https://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-aarch64-O0-g/9663/consoleFull#-134330334249ba4694-19c4-4d7e-bec5-911270d8a58c

The failure should be re-producible by building the following C++ file from llvm-test-suite:

bin/clang++  -DNDEBUG  -B /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin    -Wno-unused-command-line-argument -mllvm -verify-machineinstrs -O0 -g -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.5.sdk   -w -Werror=date-time -MD -MT SingleSource/UnitTests/CMakeFiles/block-byref-cxxobj-test.dir/block-byref-cxxobj-test.cpp.o -MF SingleSource/UnitTests/CMakeFiles/block-byref-cxxobj-test.dir/block-byref-cxxobj-test.cpp.o.d -o SingleSource/UnitTests/CMakeFiles/block-byref-cxxobj-test.dir/block-byref-cxxobj-test.cpp.o -c /Users/buildslave/jenkins/workspace/test-suite-verify-machineinstrs-aarch64-O0-g/test-suite/SingleSource/UnitTests/block-byref-cxxobj-test.cpp

Hi, this caused compiler crash with error "Assertion `cast<DISubprogram>(Scope)->describes(&MF->getFunction())' failed." on iOS platforms. So, I reverted it.
I'm working on a reduced repro.

FYI this also caused a failure on GreenDragon, with -verify-machineinstrs: https://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-aarch64-O0-g/9663/consoleFull#-134330334249ba4694-19c4-4d7e-bec5-911270d8a58c

The failure should be re-producible by building the following C++ file from llvm-test-suite:

bin/clang++  -DNDEBUG  -B /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin    -Wno-unused-command-line-argument -mllvm -verify-machineinstrs -O0 -g -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.5.sdk   -w -Werror=date-time -MD -MT SingleSource/UnitTests/CMakeFiles/block-byref-cxxobj-test.dir/block-byref-cxxobj-test.cpp.o -MF SingleSource/UnitTests/CMakeFiles/block-byref-cxxobj-test.dir/block-byref-cxxobj-test.cpp.o.d -o SingleSource/UnitTests/CMakeFiles/block-byref-cxxobj-test.dir/block-byref-cxxobj-test.cpp.o -c /Users/buildslave/jenkins/workspace/test-suite-verify-machineinstrs-aarch64-O0-g/test-suite/SingleSource/UnitTests/block-byref-cxxobj-test.cpp

Thanks.

Thank you for the repro. I added calls to ApplyDebugLocation::CreateArtificial and reapplied the patch in https://github.com/llvm/llvm-project/commit/8db0dbbe2c0544c38f33cf64b4cdd5135d524b23.