This is an archive of the discontinued LLVM Phabricator instance.

[clang][IFS] Prevent Clang-IFS from Leaking symbols from inside a block.
ClosedPublic

Authored by plotfi on Dec 10 2019, 1:43 PM.

Details

Summary

Built libdispatch with clang interface stubs. Ran into some block related issues. Basically VarDecl symbols can leak out because I wasn't checking the case where a VarDecl is contained inside a BlockDecl (versus a method or function).

This patch checks that a VarDecl is not a child decl of a BlockDecl.
This patch also does something very similar for c++ lambdas as well.

Diff Detail

Event Timeline

plotfi created this revision.Dec 10 2019, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 1:43 PM

Should probably add a check for __block variables.

plotfi updated this revision to Diff 233435.Dec 11 2019, 12:33 PM
plotfi edited the summary of this revision. (Show Details)

Should probably add a check for __block variables.

I looked into it, I am not sure if anything special needs to be done for block. ie:

echo "void f() { __block int x = 42; void (^sum)(int) = ^(int y) { x = x + y; }; }" | clang -x c -fblocks -o - - -c  | llvm-nm -
                 U _Block_object_assignecho "void f() { __block int x = 42; void (^sum)(int) = ^(int y) { x = x + y; }; }" | clang -x c -fblocks -o - - -c  | llvm-nm -
                 U _Block_object_assign
                 U _Block_object_dispose
                 U _NSConcreteStackBlock
0000000000000000 r __block_descriptor_tmp
00000000000000c0 W __copy_helper_block_8_32r
00000000000000f0 W __destroy_helper_block_8_32r
0000000000000090 t __f_block_invoke
0000000000000000 T 
                 U _Block_object_dispose
                 U _NSConcreteStackBlock
0000000000000000 r __block_descriptor_tmp
00000000000000c0 W __copy_helper_block_8_32r
00000000000000f0 W __destroy_helper_block_8_32r
0000000000000090 t __f_block_invoke
0000000000000000 T
compnerd added inline comments.Dec 16 2019, 8:34 PM
clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp
60

Use isa rather than dyn_cast_or_null. Furthermore, the dyn_cast_or_null is wrong - Parent is guaranteed to be !NULL given the condition on L56.

if (const auto *VD = dyn_cast<VarDecl>(ND))
  if (const auto *P = VD->getParentFunctionOrMethod())
    if (isa<BlockDecl>(P) || isa<CXXMethodDecl>(P))
      return true;
plotfi updated this revision to Diff 237739.Jan 13 2020, 11:27 AM

using isa

plotfi updated this revision to Diff 237740.Jan 13 2020, 11:28 AM
plotfi marked an inline comment as done.
compnerd accepted this revision.Jan 13 2020, 11:48 AM
This revision is now accepted and ready to land.Jan 13 2020, 11:48 AM
plotfi updated this revision to Diff 237752.Jan 13 2020, 12:26 PM

Going to do an NFC pre-commit before this to add the braces.

thakis added a subscriber: thakis.Jan 13 2020, 1:10 PM

This breaks tests on macOS: http://45.33.8.238/mac/5672/step_7.txt (macOS prefixes functions with an extra _.)

This breaks tests on macOS: http://45.33.8.238/mac/5672/step_7.txt (macOS prefixes functions with an extra _.)

Thank You Nico! Will fix

This breaks tests on macOS: http://45.33.8.238/mac/5672/step_7.txt (macOS prefixes functions with an extra _.)

Should be fixed now. Sorry about this. Thanks Again Nico.

This test fails on Windows: http://45.33.8.238/win/5658/step_7.txt

Looks like this expects itanium mangling; maybe you want -triple %itanium_abi_triple?

(And thanks for the mac fix!)

This test fails on Windows: http://45.33.8.238/win/5658/step_7.txt

Looks like this expects itanium mangling; maybe you want -triple %itanium_abi_triple?

Since the bot's been red for a while now, I went ahead and landed that fix in 84baf123. The bot's green now, but please check if that's what the test wants to test or if this just papers over things.

This test fails on Windows: http://45.33.8.238/win/5658/step_7.txt

Looks like this expects itanium mangling; maybe you want -triple %itanium_abi_triple?

Since the bot's been red for a while now, I went ahead and landed that fix in 84baf123. The bot's green now, but please check if that's what the test wants to test or if this just papers over things.

Thank you for this!

This test fails on Windows: http://45.33.8.238/win/5658/step_7.txt

Looks like this expects itanium mangling; maybe you want -triple %itanium_abi_triple?

I missed this message. Sorry about that. I'll remember to use %itanium_abi_triple in the future (didn't know that was a thing, was just using the linux triple if I wanted itanium abi). Thanks again.

Hi @plotfi, this seems to cause failures with ASAN and UBSAN on green dragon: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6886/consoleFull

The following tests are failing:

Clang :: InterfaceStubs/driver-test.c
Clang :: InterfaceStubs/driver-test2.c

Thanks for pinging on this @thegameg. I had been looking at this a day or two ago but got side-tracked. Will take another look.

Hi @plotfi, this seems to cause failures with ASAN and UBSAN on green dragon: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6886/consoleFull

The following tests are failing:

Clang :: InterfaceStubs/driver-test.c
Clang :: InterfaceStubs/driver-test2.c

This appears to only happen with -fintegrated-cc1, with fno-integrated-cc1 it does not repro. Still looking into this.

Hi @plotfi, this seems to cause failures with ASAN and UBSAN on green dragon: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6886/consoleFull

The following tests are failing:

Clang :: InterfaceStubs/driver-test.c
Clang :: InterfaceStubs/driver-test2.c

So, the culprit appears to be D69825. The way InterfaceStubs assembles the pipeline appears to trigger an asan bug. For now I will alter the tests to get the bots green, but once I get my bugzilla access sorted I intend to file a bug. -PL

Hi @plotfi, this seems to cause failures with ASAN and UBSAN on green dragon: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6886/consoleFull

The following tests are failing:

Clang :: InterfaceStubs/driver-test.c
Clang :: InterfaceStubs/driver-test2.c

So, the culprit appears to be D69825. The way InterfaceStubs assembles the pipeline appears to trigger an asan bug. For now I will alter the tests to get the bots green, but once I get my bugzilla access sorted I intend to file a bug. -PL

Hi @plotfi, this seems to cause failures with ASAN and UBSAN on green dragon: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6886/consoleFull

The following tests are failing:

Clang :: InterfaceStubs/driver-test.c
Clang :: InterfaceStubs/driver-test2.c

Great, thanks for taking a look!