[BuildLibCalls] Add noalias for strcat and stpcpy

[xbolva00]

Summary

strcat:
destination and source shall not overlap. (http://www.cplusplus.com/reference/cstring/strcat/)

stpcpy:
The strings may not overlap, and the destination string dest must be large enough to receive the copy. (https://man7.org/linux/man-pages/man3/stpcpy.3.html)

[fhahn]

LGTM, I checked libc on macOS which also states that overlapping string args are UB for both strcpy & strcat. It seems a bit odd that those were missed out initially, so please wait a bit with committing, in case there's something I am missing.

[efriedma]

It's conceivable that we might not want to mark strcpy/stpcpy etc. noalias for the same reason llvm.memcpy isn't noalias: standard implementation tolerate exact overlap, and the benefit from forbidding that is small. (Actually, I'm not sure any optimizations benefit at the moment.) Maybe that's being overconservative, though.

(This doesn't apply to strcat.)

[xbolva00]

That would be really overconservative

some tools and gcc warns and exploits this restriction.

[fhahn]

In D88335#2295872, @efriedma wrote:

It's conceivable that we might not want to mark strcpy/stpcpy etc. noalias for the same reason llvm.memcpy isn't noalias: standard implementation tolerate exact overlap, and the benefit from forbidding that is small. (Actually, I'm not sure any optimizations benefit at the moment.) Maybe that's being overconservative, though.

Hmm, it appears we already use noalias for strcpy/strncpy and this patch only additionally adds it to strcat/stpcpy.

noalias on its own is probably not too useful in this context, but I think without noalias, we cannot mark either argument as readonly/writeonly, which prevents some DSE opportunities (https://bugs.llvm.org/show_bug.cgi?id=47644). IIRC the main reason to allow exact overlap for llvm.memcpy was because clang generated code that violated the assumption. I think for the memcpy libfunc we still add noalias for both arguments.

[efriedma]

If everyone is satisfied the noalias markings won't cause any practical issues, that's fine.

noalias on its own is probably not too useful in this context, but I think without noalias, we cannot mark either argument as readonly/writeonly

That isn't how readonly works. From LangRef: "On an argument, this attribute indicates that the function does not write through this pointer argument, even though it may write to the memory that the pointer points to."

[fhahn]

In D88335#2295923, @efriedma wrote:

noalias on its own is probably not too useful in this context, but I think without noalias, we cannot mark either argument as readonly/writeonly

That isn't how readonly works. From LangRef: "On an argument, this attribute indicates that the function does not write through this pointer argument, even though it may write to the memory that the pointer points to."

Oh right, I forgot about that exception.

[jdoerfert]

FWIW, noalias on these arguments is, by itself, not doing anything. It does something if you know how much memory is accessed by the function through such an argument *for sure* and what other memory is accessed for sure. Such memory regions are not overlapping. (This is way too vague actually but I guess it doesn't matter for the gist.)

[xbolva00]

In D88335#2296000, @jdoerfert wrote:

FWIW, noalias on these arguments is, by itself, not doing anything. It does something if you know how much memory is accessed by the function through such an argument *for sure* and what other memory is accessed for sure. Such memory regions are not overlapping. (This is way too vague actually but I guess it doesn't matter for the gist.)

so, the patch as is, is OK for you all?

[jdoerfert]

LGTM

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

bool llvm::inferLibFuncAttributes(Function &F, const TargetLibraryInfo &TLI) {
   case LibFunc_strtold:
   case LibFunc_strtoull:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 0);
     return Changed;
   case LibFunc_strcpy:
   case LibFunc_strncpy:
-    Changed |= setDoesNotAlias(F, 0);
-    Changed |= setDoesNotAlias(F, 1);
-    LLVM_FALLTHROUGH;
   case LibFunc_strcat:
   case LibFunc_strncat:
     Changed |= setReturnedArg(F, 0);
     LLVM_FALLTHROUGH;
   case LibFunc_stpcpy:
   case LibFunc_stpncpy:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 1);
+    Changed |= setDoesNotAlias(F, 0);
+    Changed |= setDoesNotAlias(F, 1);
     return Changed;
   case LibFunc_strxfrm:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 1);
     return Changed;
   case LibFunc_strcmp:      // 0,1

llvm/test/Transforms/InferFunctionAttrs/annotate.ll

 declare i32 @stat64(i8*, %opaque*)
 
 ; CHECK: declare noundef i32 @statvfs(i8* nocapture noundef readonly, %opaque* nocapture noundef) [[G1]]
 declare i32 @statvfs(i8*, %opaque*)
 
 ; CHECK-LINUX: declare noundef i32 @statvfs64(i8* nocapture noundef readonly, %opaque* nocapture noundef) [[G1]]
 declare i32 @statvfs64(i8*, %opaque*)
 
-; CHECK: declare i8* @stpcpy(i8*, i8* nocapture readonly) [[G1]]
+; CHECK: declare i8* @stpcpy(i8* noalias, i8* noalias nocapture readonly) [[G1]]
 declare i8* @stpcpy(i8*, i8*)
 
-; CHECK: declare i8* @stpncpy(i8*, i8* nocapture readonly, i64) [[G1]]
+; CHECK: declare i8* @stpncpy(i8* noalias, i8* noalias nocapture readonly, i64) [[G1]]
 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]]
+; CHECK: declare i8* @strcat(i8* noalias returned, i8* noalias nocapture readonly) [[G1]]
 declare i8* @strcat(i8*, i8*)
 
 ; CHECK: declare i8* @strchr(i8*, i32) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i8* @strchr(i8*, i32)
 
 ; CHECK: declare i32 @strcmp(i8* nocapture, i8* nocapture) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i32 @strcmp(i8*, i8*)
 

 declare i8* @strdup(i8*)
 
 ; 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]]
+; CHECK: declare i8* @strncat(i8* noalias returned, i8* noalias nocapture readonly, i64) [[G1]]
 declare i8* @strncat(i8*, i8*, i64)
 
 ; 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)