[PATCH] D49816: [GlobalOpt] Test array indices inside structs for out-of-bounds accesses

[Dave Green via Phabricator via llvm-commits]

dmgreen created this revision.
dmgreen added reviewers: efriedma, rsmith, hfinkel.

First go at fixing PR38309.  We now, from clang, can turn arrays of

  static short g_data[] = {16, 16, 16, 16, 16, 16, 16, 16, 0, 0, 0, 0, 0, 0, 0, 0};

into structs of the form

  @g_data = internal global <{ [8 x i16], [8 x i16] }> ...

GlobalOpt can incorrectly SROA it, not realising that the access to the first
element may overflow into the second.

I believe this makes the globalsra-partial.ll test case invalid as the %i value
could be out of bounds. I've re-purposed it as a negative test for this case.


https://reviews.llvm.org/D49816

Files:
  lib/Transforms/IPO/GlobalOpt.cpp
  test/Transforms/GlobalOpt/globalsra-partial.ll


Index: test/Transforms/GlobalOpt/globalsra-partial.ll
===================================================================
--- test/Transforms/GlobalOpt/globalsra-partial.ll
+++ test/Transforms/GlobalOpt/globalsra-partial.ll
@@ -1,11 +1,12 @@
-; In this case, the global can only be broken up by one level.
+; In this case, the global cannot be merged as i may be out of range
 
 ; RUN: opt < %s -globalopt -S | FileCheck %s
 target datalayout = "E-p:64:64:64-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128"
 
 @G = internal global { i32, [4 x float] } zeroinitializer               ; <{ i32, [4 x float] }*> [#uses=3]
 
-; CHECK-NOT: 12345
+; CHECK: @G = internal unnamed_addr global { i32, [4 x float] }
+; CHECK: 12345
 define void @onlystore() {
         store i32 12345, i32* getelementptr ({ i32, [4 x float] }, { i32, [4 x float] }* @G, i32 0, i32 0)
         ret void
Index: lib/Transforms/IPO/GlobalOpt.cpp
===================================================================
--- lib/Transforms/IPO/GlobalOpt.cpp
+++ lib/Transforms/IPO/GlobalOpt.cpp
@@ -409,8 +409,9 @@
   gep_type_iterator GEPI = gep_type_begin(U), E = gep_type_end(U);
   ++GEPI;  // Skip over the pointer index.
 
-  // If this is a use of an array allocation, do a bit more checking for sanity.
-  if (GEPI.isSequential()) {
+  // If this is a use of an array allocation (possibly inside a struct), do a
+  // bit more checking for sanity.
+  if (GEPI.isSequential() || GEPI.isStruct()) {
     ConstantInt *Idx = cast<ConstantInt>(U->getOperand(2));
 
     // Check to make sure that index falls within the array.  If not,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D49816.157333.patch
Type: text/x-patch
Size: 1648 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180725/7f6efbe1/attachment.bin>