[Analysis] Be defensive when matching size_t in lib call signatures

[bjope]

Summary

When TargetLibraryInfoImpl::isValidProtoForLibFunc is checking
function signatures to detect lib calls it may check that a parameter
or return value matches with the "size_t" type. For this to work it
has to derive the IR type matching with "size_t". Depending on if
a DataLayout is provided or not, this has been done in two different
way. Either a more strict check being based on IntPtrType (which is
given by the DataLayout) or a more relaxed check assuming that any
integer type matches with "size_t".

Given that the stricter approach exist it seems like we do not want
to trigger rewrites etc if we aren't sure that a function calls
actually match with the library function. Therefore it was questioned
why we actually have the more relaxed approach when not being able
to derive an IR type for "size_t". This patch will take a more
defensive approach, avoiding lib call transformations when we do not
know if the signature matches (when we do not know the size of
"size_t").

[nikic]

I think the "optional DataLayout" part here is just an unused leftover. If I add an assert(DL) in getLibFunc() (the only caller of isValidProtoForLibFunc()) I don't get any test failures. I'd recommend just making DataLayout explicitly required. Assert / make it a reference.

[nikic]

llvm/lib/Analysis/TargetLibraryInfo.cpp

734

As a side note, I believe this should really be using the integer type with width DL.getIndexSizeInBits(). Basically pointer size = intptr_t and index size = size_t.

[bjope]

llvm/lib/Analysis/TargetLibraryInfo.cpp

734

Ok. I did not know about "index size = size_t" relation.

(Makes me wonder about the index type in the DataLayout for our OOT target. We haven't configured that afaict, so we get index size = pointer size by default. But in our target size_t is different from intptr_t. So we should probably specify those optional params in the data layout to set the IndexSize. Will be interesting to see if that will impact something else.)

[bjope]

Update after review feedback:
Now assuming that we always have a Module (and DataLayout) when using getLibFunc/isValidProtoForLibFunc.

[nikic]

LG

llvm/lib/Analysis/TargetLibraryInfo.cpp

 //===-- TargetLibraryInfo.cpp - Runtime library information ----------------==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 }
 
 bool TargetLibraryInfoImpl::isValidProtoForLibFunc(const FunctionType &FTy,
                                                    LibFunc F,
                                                    const DataLayout *DL) const {
   LLVMContext &Ctx = FTy.getContext();
   // FIXME: There is really no guarantee that sizeof(size_t) is equal to
   // sizeof(int*) for every target. So the assumption used here to derive the
   // SizeTTy based on DataLayout and getIntPtrType isn't always valid.
   Type *SizeTTy = DL ? DL->getIntPtrType(Ctx, /*AddressSpace=*/0) : nullptr;
   auto IsSizeTTy = [SizeTTy](Type *Ty) {
-    // FIXME: For uknown historical reasons(?) we use a relaxed condition saying
-    // that any integer type may size_t, for example if we got no
-    // DataLayout. This seems like a potentially error prone relaxation (or why
-    // should we only be more strict and checking the exact type when we have a
-    // DataLayout?).
     if (!SizeTTy)
-      return Ty->isIntegerTy();
+      return false;
     return Ty == SizeTTy;
   };
   unsigned NumParams = FTy.getNumParams();
 
   switch (F) {
   case LibFunc_execl:
   case LibFunc_execlp:
   case LibFunc_execle: