Page MenuHomePhabricator

[NOT FOR REVIEW] Experimental support for zero-or-trap behavior for uninitialized variables.
Needs ReviewPublic

Authored by rsmith on May 1 2020, 10:30 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Add <type> @llvm.trapping.<type>(<type>) intrinsic.

This is intended to produce either the given value or, if we can trace a
use of the value to an instruction with side-effects, a trap.
Notionally, it behaves as a nondeterministic choice between the given
value and poison, where the choice is made angelically such that the
given value is always chosen when the program does not reach a trap.

Simplify trapping(x) and convert it into traps where possible.

Update Clang CodeGen to produce trapping(x) in trivial auto var init
mode.

Diff Detail

Unit TestsFailed

TimeTest
1,170 msClang.CodeGenCXX::auto-var-init.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -std=c++14 -triple x86_64-unknown-unknown -fblocks /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/auto-var-init.cpp -emit-llvm -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/auto-var-init.cpp -check-prefixes=CHECK,CHECK-O0
480 msClang.CodeGenCXX::trivial-auto-var-init.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -triple x86_64-unknown-unknown -fblocks /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/trivial-auto-var-init.cpp -emit-llvm -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/trivial-auto-var-init.cpp -check-prefix=UNINIT

Event Timeline

rsmith created this revision.May 1 2020, 10:30 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 1 2020, 10:30 AM
rsmith updated this revision to Diff 261511.May 1 2020, 11:37 AM
  • The second operand of 'store' is the pointer, not the first.
srhines added a subscriber: srhines.May 6 2020, 8:14 PM
jfb added a subscriber: jfb.Wed, Jun 17, 5:10 PM

Overall I like this approach.

I think it needs three more things to make it:

  • Better size optimizations. I measured the code size impact on a codebase which deploys variable auto-init already, and it's a 0.5% code size hit. Considering that auto-init itself was 1%, it means the mitigation now costs 50% more. I haven't dug into why the increase is such, and I assume that there's low-lying fruits.
  • I haven't measure performance impact. It might be zero.
  • I think we'd need opt remarks support, because we'd want to audit all the places where a trap is left behind.
clang/lib/CodeGen/CGDecl.cpp
1166

Here you need something like:

auto *PointerTy = Ty->getPointerTo(Loc.getAddressSpace());
if (V->getType() != PointerTy)
  Loc = Builder.CreateBitCast(Loc, PointerTy);

Because you can be storing a constant which is layout-compatible with the location, but which doesn't have the same type (say, when we have an explicit padding array of bytes). Line 1184 does that already for other code paths.

FWIW, I tried to solve something similar the other day. My solution sketch looked like this: https://godbolt.org/z/bRQPjd
The idea would be that we teach DSE (and others) to remove the llvm.undef.init intrinsic if the location is overwritten.
In the example above only SROA kicks in properly. I guess this solution is more powerful though.