[BuildLibCalls] Add argmemonly to more lib calls.

[fhahn]

Summary

strspn, strncmp, strcspn, strcasecmp, strncasecmp, memcmp, memchr,
memrchr, memcpy, memmove, memcpy, mempcpy should all only access memory
through their arguments.

I broke out strcoll, because the result depends on the locale, which
might get accessed through memory.

I also added nofree to memcmp, so we do not introduce another attribute
group for in the tests.

[xbolva00]

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

188–189

strchr too?

321–322

Same for bcmp?

329

We miss attrs for strnlen

[xbolva00]

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

325

Should be nofree opt out rather than opt in?

@jeoerfert

[xbolva00]

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

325

Ok, we have there:

if(!isLibFreeFunction(&F, TheLibFunc) && !isReallocLikeFn(&F, &TLI))

Changed |= setDoesNotFreeMemory(F);

So why we need to set nofree for memcmp?

[jdoerfert]

FWIW, I have a non-clean patch for this too that has some more things we could add: https://github.com/jdoerfert/llvm-project/commit/c405073b6f353b3bccb10f222185763c4c7e3457
I would recommend we sort the order in each case, arguments first, in order of their number, than return than function, or something similar. that makes it easier to compare and read IMHO.
We should maybe wait for D85932 and start using the script for this file, it can handle what we need it too nicely (I think/hope).

I'm fine with doing them in multiple steps, especially D85932 should make this even simpler.

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

325

I think it was set before already. So no need to add it here if that was the case.

[efriedma]

I broke out strcoll, because the result depends on the locale, which might get accessed through memory.

Aren't strcasecmp/strncasecmp also locale-sensitive?

[jdoerfert]

In D86724#2242825, @efriedma wrote:

I broke out strcoll, because the result depends on the locale, which might get accessed through memory.

Aren't strcasecmp/strncasecmp also locale-sensitive?

Hm, not clear to me:

The POSIX.1-2008 standard says of these functions:
 
       When the LC_CTYPE category of the locale being used is from  the
       POSIX locale, these functions shall behave as if the strings had
       been converted to lowercase and  then  a  byte  comparison  per‐
       formed.  Otherwise, the results are unspecified.

[efriedma]

https://www.gnu.org/software/libc/manual/html_node/String_002fArray-Comparison.html#index-strcasecmp says "How uppercase and lowercase characters are related is determined by the currently selected locale." That seems pretty clear.

[fhahn]

Thanks, updated to add argmemonly also for strchr,bcmp. Do not add it for strcasecmp/strncasecmp. Remove unnecessary setDoesNotFreeMemory.

In D86724#2242628, @jdoerfert wrote:

FWIW, I have a non-clean patch for this too that has some more things we could add: https://github.com/jdoerfert/llvm-project/commit/c405073b6f353b3bccb10f222185763c4c7e3457
I would recommend we sort the order in each case, arguments first, in order of their number, than return than function, or something similar. that makes it easier to compare and read IMHO.
We should maybe wait for D85932 and start using the script for this file, it can handle what we need it too nicely (I think/hope).

That's great, there's definitely lots of additional stuff missing. I already adjusted the lines, I don't think there a need to await for a script update. We can just re-generate the lines once the script update is in.

I'm fine with doing them in multiple steps, especially D85932 should make this even simpler.

I think it is beneficial to do this in a few steps, so we can more easily bisect problems.

In D86724#2242875, @efriedma wrote:

https://www.gnu.org/software/libc/manual/html_node/String_002fArray-Comparison.html#index-strcasecmp says "How uppercase and lowercase characters are related is determined by the currently selected locale." That seems pretty clear.

Thanks, this indeed seems clear. The first page I found was not as explicit https://linux.die.net/man/3/strcasecmp. I moved them to strcoll.

[fhahn]

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

325

Oh right, that's great, I removed it.

329

probably best to add in a separate patch, as it is completely missing?

[jdoerfert]

LGTM

[fhahn]

Thanks!

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

bool llvm::inferLibFuncAttributes(Function &F, const TargetLibraryInfo &TLI) {
   case LibFunc_strlen:
   case LibFunc_wcslen:
     Changed |= setOnlyReadsMemory(F);
     Changed |= setDoesNotThrow(F);
     Changed |= setOnlyAccessesArgMemory(F);
     Changed |= setDoesNotCapture(F, 0);
     return Changed;
   case LibFunc_strchr:
   case LibFunc_strrchr:
+    Changed |= setOnlyAccessesArgMemory(F);
     Changed |= setOnlyReadsMemory(F);
     Changed |= setDoesNotThrow(F);
     return Changed;
   case LibFunc_strtol:
   case LibFunc_strtod:
   case LibFunc_strtof:
   case LibFunc_strtoul:
   case LibFunc_strtoll:
case LibFunc_strxfrm:
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 1);
     return Changed;
   case LibFunc_strcmp:      // 0,1
   case LibFunc_strspn:      // 0,1
   case LibFunc_strncmp:     // 0,1
   case LibFunc_strcspn:     // 0,1
-  case LibFunc_strcoll:     // 0,1
+    Changed |= setDoesNotThrow(F);
+    Changed |= setOnlyAccessesArgMemory(F);
+    Changed |= setOnlyReadsMemory(F);
+    Changed |= setDoesNotCapture(F, 0);
+    Changed |= setDoesNotCapture(F, 1);
+    return Changed;
+  case LibFunc_strcoll:
   case LibFunc_strcasecmp:  // 0,1
   case LibFunc_strncasecmp: //
+    // Those functions may depend on the locale, which may be accessed through
+    // global memory.
     Changed |= setOnlyReadsMemory(F);
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setDoesNotCapture(F, 1);
     return Changed;
   case LibFunc_strstr:
   case LibFunc_strpbrk:
     Changed |= setOnlyReadsMemory(F);
case LibFunc_system:
     // May throw; "system" is a valid pthread cancellation point.
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setOnlyReadsMemory(F, 0);
     return Changed;
   case LibFunc_malloc:
     Changed |= setDoesNotThrow(F);
     Changed |= setRetDoesNotAlias(F);
     return Changed;
   case LibFunc_memcmp:
+    Changed |= setOnlyAccessesArgMemory(F);
     Changed |= setOnlyReadsMemory(F);
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setDoesNotCapture(F, 1);
     return Changed;
   case LibFunc_memchr:
   case LibFunc_memrchr:
-    Changed |= setOnlyReadsMemory(F);
     Changed |= setDoesNotThrow(F);
+    Changed |= setOnlyAccessesArgMemory(F);
+    Changed |= setOnlyReadsMemory(F);
     return Changed;
   case LibFunc_modf:
   case LibFunc_modff:
   case LibFunc_modfl:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 1);
     return Changed;
   case LibFunc_memcpy:
+    Changed |= setDoesNotThrow(F);
+    Changed |= setOnlyAccessesArgMemory(F);
     Changed |= setDoesNotAlias(F, 0);
-    Changed |= setDoesNotAlias(F, 1);
     Changed |= setReturnedArg(F, 0);
-    Changed |= setDoesNotThrow(F);
+    Changed |= setDoesNotAlias(F, 1);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 1);
     return Changed;
   case LibFunc_memmove:
-    Changed |= setReturnedArg(F, 0);
     Changed |= setDoesNotThrow(F);
+    Changed |= setOnlyAccessesArgMemory(F);
+    Changed |= setReturnedArg(F, 0);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 1);
     return Changed;
   case LibFunc_mempcpy:
   case LibFunc_memccpy:
+    Changed |= setDoesNotThrow(F);
+    Changed |= setOnlyAccessesArgMemory(F);
     Changed |= setDoesNotAlias(F, 0);
     Changed |= setDoesNotAlias(F, 1);
-    Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 1);
     return Changed;
   case LibFunc_memcpy_chk:
     Changed |= setDoesNotThrow(F);
     return Changed;
   case LibFunc_memalign:
     Changed |= setRetDoesNotAlias(F);
bool llvm::inferLibFuncAttributes(Function &F, const TargetLibraryInfo &TLI) {
   case LibFunc_bcopy:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 0);
     return Changed;
   case LibFunc_bcmp:
     Changed |= setDoesNotThrow(F);
+    Changed |= setOnlyAccessesArgMemory(F);
     Changed |= setOnlyReadsMemory(F);
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setDoesNotCapture(F, 1);
     return Changed;
   case LibFunc_bzero:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 0);
     return Changed;

llvm/test/Transforms/InferFunctionAttrs/annotate.ll

 declare i32 @atoi(i8*)
 
 ; CHECK: declare i64 @atol(i8* nocapture) [[G2]]
 declare i64 @atol(i8*)
 
 ; CHECK: declare i64 @atoll(i8* nocapture) [[G2]]
 declare i64 @atoll(i8*)
 
-; CHECK-LINUX: declare i32 @bcmp(i8* nocapture, i8* nocapture, i64) [[G2]]
+; CHECK-LINUX: declare i32 @bcmp(i8* nocapture, i8* nocapture, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY:#[0-9]+]]
 ; CHECK-DARWIN-NOT: declare i32 @bcmp(i8* nocapture, i8* nocapture, i64) [[G2]]
 ; CHECK-UNKNOWN-NOT: declare i32 @bcmp(i8* nocapture, i8* nocapture, i64) [[G2]]
 ; CHECK-NVPTX-NOT: declare i32 @bcmp(i8* nocapture, i8* nocapture, i64) [[G2]]
 declare i32 @bcmp(i8*, i8*, i64)
 
 ; CHECK: declare void @bcopy(i8* nocapture readonly, i8* nocapture, i64) [[G1]]
 declare void @bcopy(i8*, i8*, i64)
 

 declare i32 @lstat64(i8*, %opaque*)
 
 ; CHECK: declare noalias i8* @malloc(i64) [[G1]]
 declare i8* @malloc(i64)
 
 ; CHECK-LINUX: declare noalias i8* @memalign(i64, i64) [[G0]]
 declare i8* @memalign(i64, i64)
 
-; CHECK: declare i8* @memccpy(i8* noalias, i8* noalias nocapture readonly, i32, i64) [[G1]]
+; CHECK: declare i8* @memccpy(i8* noalias, i8* noalias nocapture readonly, i32, i64) [[ARGMEMONLY_NOFREE_NOUNWIND:#[0-9]+]]
 declare i8* @memccpy(i8*, i8*, i32, i64)
 
-; CHECK: declare i8* @memchr(i8*, i32, i64) [[G2]]
+; CHECK-LINUX:   declare i8* @memchr(i8*, i32, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
+; CHECK-DARWIN:  declare i8* @memchr(i8*, i32, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY:#[0-9]+]]
+; CHECK-UNKNOWN: declare i8* @memchr(i8*, i32, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY:#[0-9]+]]
 declare i8* @memchr(i8*, i32, i64)
 
-; CHECK: declare i32 @memcmp(i8* nocapture, i8* nocapture, i64) [[G2]]
+; CHECK: declare i32 @memcmp(i8* nocapture, i8* nocapture, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i32 @memcmp(i8*, i8*, i64)
 
-; CHECK: declare i8* @memcpy(i8* noalias returned, i8* noalias nocapture readonly, i64) [[G1]]
+; CHECK: declare i8* @memcpy(i8* noalias returned, i8* noalias nocapture readonly, i64) [[ARGMEMONLY_NOFREE_NOUNWIND]]
 declare i8* @memcpy(i8*, i8*, i64)
 
-; CHECK: declare i8* @mempcpy(i8* noalias, i8* noalias nocapture readonly, i64) [[G1]]
+; CHECK: declare i8* @mempcpy(i8* noalias, i8* noalias nocapture readonly, i64) [[ARGMEMONLY_NOFREE_NOUNWIND]]
 declare i8* @mempcpy(i8*, i8*, i64)
 
-; CHECK: declare i8* @memmove(i8* returned, i8* nocapture readonly, i64) [[G1]]
+; CHECK: declare i8* @memmove(i8* returned, i8* nocapture readonly, i64) [[ARGMEMONLY_NOFREE_NOUNWIND]]
 declare i8* @memmove(i8*, i8*, i64)
 
 ; CHECK: declare i8* @memset(i8*, i32, i64) [[G0]]
 declare i8* @memset(i8*, i32, i64)
 
 ; CHECK: declare i32 @mkdir(i8* nocapture readonly, i16 zeroext) [[G1]]
 declare i32 @mkdir(i8*, i16 zeroext)
 

 declare i8* @stpncpy(i8*, i8*, i64)
 
 ; CHECK: declare i32 @strcasecmp(i8* nocapture, i8* nocapture) [[G2]]
 declare i32 @strcasecmp(i8*, i8*)
 
 ; CHECK: declare i8* @strcat(i8* returned, i8* nocapture readonly) [[G1]]
 declare i8* @strcat(i8*, i8*)
 
-; CHECK: declare i8* @strchr(i8*, i32) [[G2]]
+; CHECK: declare i8* @strchr(i8*, i32) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i8* @strchr(i8*, i32)
 
-; CHECK: declare i32 @strcmp(i8* nocapture, i8* nocapture) [[G2]]
+; CHECK: declare i32 @strcmp(i8* nocapture, i8* nocapture) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i32 @strcmp(i8*, i8*)
 
 ; CHECK: declare i32 @strcoll(i8* nocapture, i8* nocapture) [[G2]]
 declare i32 @strcoll(i8*, i8*)
 
 ; CHECK: declare i8* @strcpy(i8* noalias returned, i8* noalias nocapture readonly) [[G1]]
 declare i8* @strcpy(i8*, i8*)
 
-; CHECK: declare i64 @strcspn(i8* nocapture, i8* nocapture) [[G2]]
+; CHECK: declare i64 @strcspn(i8* nocapture, i8* nocapture) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i64 @strcspn(i8*, i8*)
 
 ; CHECK: declare noalias i8* @strdup(i8* nocapture readonly) [[G1]]
 declare i8* @strdup(i8*)
 
-; CHECK: declare i64 @strlen(i8* nocapture) [[G4:#[0-9]+]]
+; CHECK: declare i64 @strlen(i8* nocapture) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i64 @strlen(i8*)
 
 ; CHECK: declare i32 @strncasecmp(i8* nocapture, i8* nocapture, i64) [[G2]]
 declare i32 @strncasecmp(i8*, i8*, i64)
 
 ; CHECK: declare i8* @strncat(i8* returned, i8* nocapture readonly, i64) [[G1]]
 declare i8* @strncat(i8*, i8*, i64)
 
-; CHECK: declare i32 @strncmp(i8* nocapture, i8* nocapture, i64) [[G2]]
+; CHECK: declare i32 @strncmp(i8* nocapture, i8* nocapture, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i32 @strncmp(i8*, i8*, i64)
 
 ; CHECK: declare i8* @strncpy(i8* noalias returned, i8* noalias nocapture readonly, i64) [[G1]]
 declare i8* @strncpy(i8*, i8*, i64)
 
 ; CHECK: declare noalias i8* @strndup(i8* nocapture readonly, i64) [[G1]]
 declare i8* @strndup(i8*, i64)
 
 ; CHECK: declare i64 @strnlen(i8*, i64) [[G0]]
 declare i64 @strnlen(i8*, i64)
 
 ; CHECK: declare i8* @strpbrk(i8*, i8* nocapture) [[G2]]
 declare i8* @strpbrk(i8*, i8*)
 
-; CHECK: declare i8* @strrchr(i8*, i32) [[G2]]
+; CHECK: declare i8* @strrchr(i8*, i32) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i8* @strrchr(i8*, i32)
 
-; CHECK: declare i64 @strspn(i8* nocapture, i8* nocapture) [[G2]]
+; CHECK: declare i64 @strspn(i8* nocapture, i8* nocapture) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i64 @strspn(i8*, i8*)
 
 ; CHECK: declare i8* @strstr(i8*, i8* nocapture) [[G2]]
 declare i8* @strstr(i8*, i8*)
 
 ; CHECK: declare double @strtod(i8* readonly, i8** nocapture) [[G1]]
 declare double @strtod(i8*, i8**)
 

 ; CHECK: declare noundef i32 @vsscanf(i8* nocapture noundef readonly, i8* nocapture noundef readonly, %opaque* noundef) [[G1]]
 declare i32 @vsscanf(i8*, i8*, %opaque*)
 
 ; CHECK: declare noundef i64 @write(i32 noundef, i8* nocapture noundef readonly, i64 noundef) [[G0]]
 declare i64 @write(i32, i8*, i64)
 
 
 ; memset_pattern16 isn't available everywhere.
-; CHECK-DARWIN: declare void @memset_pattern16(i8* nocapture, i8* nocapture readonly, i64) [[G5:#[0-9]+]]
+; CHECK-DARWIN: declare void @memset_pattern16(i8* nocapture, i8* nocapture readonly, i64) [[G6:#[0-9]+]]
 declare void @memset_pattern16(i8*, i8*, i64)
 
 ; CHECK: attributes [[G0]] = { nofree }
 ; CHECK: attributes [[G1]] = { nofree nounwind }
-; CHECK: attributes [[G2]] = { nofree nounwind readonly }
-; CHECK: attributes [[G3]] = { nounwind }
-; CHECK: attributes [[G4]] = { argmemonly nofree nounwind readonly }
-; CHECK-DARWIN: attributes [[G5]] = { argmemonly nofree }
+; CHECK-DAG: attributes [[G2]] = { nofree nounwind readonly }
+; CHECK-DAG: attributes [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]] = { argmemonly nofree nounwind readonly }
+; CHECK-DAG: attributes [[G3]] = { nounwind }
+; CHECK-DAG: attributes [[ARGMEMONLY_NOFREE_NOUNWIND]] = { argmemonly nofree nounwind }
+; CHECK-DARWIN: attributes [[G6]] = { argmemonly nofree }