[ValueTracking][SimplifyLibCalls] Fix bug in getConstantDataArrayInfo for wchar_t

[bjope]

Summary

When SimplifyLibCalls is dealing with wchar_t (e.g. optimizing wcslen)
it uses ValueTracking helpers with a CharSize/ElementSize that isn't
8, but rather 16 or 32 (to match with the size in bits of a wchar_t).

Problem I've seen is that llvm::getConstantDataArrayInfo is taking
both a "ElementSize" argument (basically indicating size of a
char/element in bits) and an "Offset" which afaict is an offset
in the unit "number of elements". Then it also use
stripAndAccumulateConstantOffsets to get a "StartIdx" which afaict
is calculated in bytes. The returned Slice.Length is based on
arithmetics that add/subtract variables that are having different
units (bytes vs elements). Most notably I think the "StartIdx" must
be scaled using the "ElementSize" to get correct results.

The symptom of the above problem can be seen in the wcslen-7.ll test
case which miscompiles.

This patch is supposed to resolve the bug by converting between
bytes and elements when needed.

[nikic]

llvm/lib/Analysis/ValueTracking.cpp

4269

I don't think this assertions holds. What happens if you have something like getelementptr i8, ptr @ws, i64 3?

[Ka-Ka]

llvm/lib/Analysis/ValueTracking.cpp

4269

I guess it is better to simply return false instead of hitting an assert like this.

[bjope]

Replaced assert by 'return false' in case the offset isn't a multiple of the ElementSize.

[nikic]

LGTM

llvm/lib/Analysis/ValueTracking.cpp

bool llvm::isGEPBasedOnPointerToString(const GEPOperator *GEP,
 
   return true;
 }
 
 // If V refers to an initialized global constant, set Slice either to
 // its initializer if the size of its elements equals ElementSize, or,
 // for ElementSize == 8, to its representation as an array of unsiged
 // char. Return true on success.
+// Offset is in the unit "nr of ElementSize sized elements".
 bool llvm::getConstantDataArrayInfo(const Value *V,
                                     ConstantDataArraySlice &Slice,
                                     unsigned ElementSize, uint64_t Offset) {
-  assert(V);
+  assert(V && "V should not be null.");
+  assert((ElementSize % 8) == 0 &&
+         "ElementSize expected to be a multiple of the size of a byte.");
+  unsigned ElementSizeInBytes = ElementSize / 8;
 
   // Drill down into the pointer expression V, ignoring any intervening
   // casts, and determine the identity of the object it references along
   // with the cumulative byte offset into it.
   const GlobalVariable *GV =
     dyn_cast<GlobalVariable>(getUnderlyingObject(V));
   if (!GV || !GV->isConstant() || !GV->hasDefinitiveInitializer())
     // Fail if V is not based on constant global object.
     return false;
 
   const DataLayout &DL = GV->getParent()->getDataLayout();
   APInt Off(DL.getIndexTypeSizeInBits(V->getType()), 0);
 
   if (GV != V->stripAndAccumulateConstantOffsets(DL, Off,
                                                  /*AllowNonInbounds*/ true))
     // Fail if a constant offset could not be determined.
     return false;
 
   uint64_t StartIdx = Off.getLimitedValue();
   if (StartIdx == UINT64_MAX)
     // Fail if the constant offset is excessive.
     return false;
 
-  Offset += StartIdx;
+  // Off/StartIdx is in the unit of bytes. So we need to convert to number of
+  // elements. Simply bail out if that isn't possible.
+  if ((StartIdx % ElementSizeInBytes) != 0)
+    return false;
 
+  Offset += StartIdx / ElementSizeInBytes;
   ConstantDataArray *Array = nullptr;
   ArrayType *ArrayTy = nullptr;
 
   if (GV->getInitializer()->isNullValue()) {
     Type *GVTy = GV->getValueType();
     uint64_t SizeInBytes = DL.getTypeStoreSize(GVTy).getFixedSize();
-    uint64_t Length = SizeInBytes / (ElementSize / 8);
+    uint64_t Length = SizeInBytes / ElementSizeInBytes;
 
     Slice.Array = nullptr;
     Slice.Offset = 0;
     // Return an empty Slice for undersized constants to let callers
     // transform even undefined library calls into simpler, well-defined
     // expressions.  This is preferable to making the calls although it
     // prevents sanitizers from detecting such calls.
     Slice.Length = Length < Offset ? 0 : Length - Offset;

llvm/test/Transforms/InstCombine/wcslen-1.ll

 ;
   %l = call i64 @wcslen(ptr @str16)
   ret i64 %l
 }
 
 @ws = constant [10 x i32] [i32 9, i32 8, i32 7, i32 6, i32 5, i32 4, i32 3, i32 2, i32 1, i32 0]
 
 ; Fold wcslen(ws + 2) => 7.
-; FIXME: This fold is faulty, result should be 7 not 1.
 define i64 @fold_wcslen_1() {
 ; CHECK-LABEL: @fold_wcslen_1(
-; CHECK-NEXT:    ret i64 1
+; CHECK-NEXT:    ret i64 7
 ;
   %p = getelementptr inbounds [10 x i32], ptr @ws, i64 0, i64 2
   %len = tail call i64 @wcslen(ptr %p)
   ret i64 %len
 }
 
 ; Should not crash on this, and no optimization expected (idea is to get into
 ; llvm::getConstantDataArrayInfo looking for an array with 32-bit elements but
-; with an offset that isn't a multiple of the element size).  FIXME: Looks a
-; bit weird. Don't think we should return 6 here.
+; with an offset that isn't a multiple of the element size).
 define i64 @no_fold_wcslen_1() {
 ; CHECK-LABEL: @no_fold_wcslen_1(
-; CHECK-NEXT:    ret i64 6
+; CHECK-NEXT:    %len = tail call i64 @wcslen(ptr nonnull getelementptr inbounds ([15 x i8], ptr @ws, i64 0, i64 3))
+; CHECK-NEXT:    ret i64 %len
 ;
   %p = getelementptr [15 x i8], ptr @ws, i64 0, i64 3
   %len = tail call i64 @wcslen(ptr %p)
   ret i64 %len
 }
 
 @s8 = constant [10 x i8] [i8 9, i8 8, i8 7, i8 6, i8 5, i8 4, i8 3, i8 2, i8 1, i8 0]