Page MenuHomePhabricator

[InstCombine] Fix miscompile on GEP+load to icmp fold (PR45210)
Needs ReviewPublic

Authored by hyeongyukim on Mar 28 2021, 9:00 PM.

Details

Summary

As noted in PR45210: https://bugs.llvm.org/show_bug.cgi?id=45210
...the bug is triggered as Eli say when sext(idx) * ElementSize overflows.

   // assume that GV is an array of 4-byte elements
   GEP = gep GV, 0, Idx // this is accessing Idx * 4
   L = load GEP
   ICI = icmp eq L, value
 =>
   ICI = icmp eq Idx, NewIdx

The foldCmpLoadFromIndexedGlobal function simplifies GEP+load operation to icmp.
And there is a problem because Idx * ElementSize can overflow.

Let's assume that the wanted value is at offset 0.
Then, there are actually four possible values for Idx to match offset 0: 0x00..00, 0x40..00, 0x80..00, 0xC0..00.
We should return true for all these values, but currently, the new icmp only returns true for 0x00..00.

This problem can be solved by masking off (trailing zeros of ElementSize) bits from Idx.

   ...
 =>
   Idx' = and Idx, 0x3F..FF
   ICI = icmp eq Idx', NewIdx

Diff Detail

Unit TestsFailed

TimeTest
2,250 msx64 debian > libarcher.races::lock-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c

Event Timeline

hyeongyukim created this revision.Mar 28 2021, 9:00 PM
hyeongyukim requested review of this revision.Mar 28 2021, 9:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2021, 9:00 PM
aqjune added a subscriber: aqjune.Apr 3 2021, 11:08 AM
aqjune added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
289

This is to avoid creating redundant expressions, right?
Could you elaborate a bit more with an example?

llvm/test/Transforms/InstCombine/load-cmp.ll
64–65

Could you leave a comment explaining why this is 32767?
Same for test10_struct_arr_noinbounds case below

I added some comments to clarify my codes.

Add simple examples at the comment.

hyeongyukim edited the summary of this revision. (Show Details)Apr 14 2021, 1:11 AM
hyeongyukim edited the summary of this revision. (Show Details)
hyeongyukim edited the summary of this revision. (Show Details)Apr 14 2021, 1:14 AM
hyeongyukim edited the summary of this revision. (Show Details)Apr 14 2021, 4:18 AM

The patch was a bit complicated because it included the sext case, so I uploaded a patch that did not take that case into account. The sext case was for removing redundant operations, so the current patch is still correct.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
289

Yes, I changed my comment.

aqjune added inline comments.Apr 14 2021, 4:53 AM
llvm/test/Transforms/InstCombine/load-cmp.ll
305

Okay, the mask '268435455' is 0x0F..FF and it makes sense because the size of struct %Foo is 16.