SimplifyLibCalls: Use the correct address space when computing integer widths. #118586
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -397,9 +397,7 @@ Value *LibCallSimplifier::emitStrLenMemCpy(Value *Src, Value *Dst, uint64_t Len, | |
|
|
||
| // We have enough information to now generate the memcpy call to do the | ||
| // concatenation for us. Make a memcpy to copy the nul byte with align = 1. | ||
| B.CreateMemCpy(CpyDst, Align(1), Src, Align(1), Len + 1); | ||
| return Dst; | ||
| } | ||
|
|
||
|
|
@@ -590,26 +588,35 @@ Value *LibCallSimplifier::optimizeStrCmp(CallInst *CI, IRBuilderBase &B) { | |
| if (Len1 && Len2) { | ||
| return copyFlags( | ||
| *CI, emitMemCmp(Str1P, Str2P, | ||
| ConstantInt::get( | ||
| DL.getIntPtrType( | ||
| CI->getContext(), | ||
| Str1P->getType()->getPointerAddressSpace()), | ||
| std::min(Len1, Len2)), | ||
| B, DL, TLI)); | ||
| } | ||
|
|
||
| // strcmp to memcmp | ||
| if (!HasStr1 && HasStr2) { | ||
| if (canTransformToMemCmp(CI, Str1P, Len2, DL)) | ||
| return copyFlags( | ||
| *CI, emitMemCmp(Str1P, Str2P, | ||
| ConstantInt::get( | ||
| DL.getIntPtrType( | ||
| CI->getContext(), | ||
| Str1P->getType()->getPointerAddressSpace()), | ||
| Len2), | ||
| B, DL, TLI)); | ||
| } else if (HasStr1 && !HasStr2) { | ||
| if (canTransformToMemCmp(CI, Str2P, Len1, DL)) | ||
| return copyFlags( | ||
| *CI, emitMemCmp(Str1P, Str2P, | ||
| ConstantInt::get( | ||
| DL.getIntPtrType( | ||
| CI->getContext(), | ||
| Str1P->getType()->getPointerAddressSpace()), | ||
| Len1), | ||
| B, DL, TLI)); | ||
| } | ||
|
|
||
| annotateNonNullNoUndefBasedOnAccess(CI, {0, 1}); | ||
|
|
@@ -677,18 +684,24 @@ Value *LibCallSimplifier::optimizeStrNCmp(CallInst *CI, IRBuilderBase &B) { | |
| Len2 = std::min(Len2, Length); | ||
| if (canTransformToMemCmp(CI, Str1P, Len2, DL)) | ||
| return copyFlags( | ||
| *CI, emitMemCmp(Str1P, Str2P, | ||
| ConstantInt::get( | ||
| DL.getIntPtrType( | ||
| CI->getContext(), | ||
| Str1P->getType()->getPointerAddressSpace()), | ||
| Len2), | ||
| B, DL, TLI)); | ||
| } else if (HasStr1 && !HasStr2) { | ||
| Len1 = std::min(Len1, Length); | ||
| if (canTransformToMemCmp(CI, Str2P, Len1, DL)) | ||
| return copyFlags( | ||
| *CI, emitMemCmp(Str1P, Str2P, | ||
| ConstantInt::get( | ||
| DL.getIntPtrType( | ||
| CI->getContext(), | ||
| Str1P->getType()->getPointerAddressSpace()), | ||
| Len1), | ||
| B, DL, TLI)); | ||
| } | ||
|
|
||
| return nullptr; | ||
|
|
@@ -722,9 +735,7 @@ Value *LibCallSimplifier::optimizeStrCpy(CallInst *CI, IRBuilderBase &B) { | |
|
|
||
| // We have enough information to now generate the memcpy call to do the | ||
| // copy for us. Make a memcpy to copy the nul byte with align = 1. | ||
| CallInst *NewCI = B.CreateMemCpy(Dst, Align(1), Src, Align(1), Len); | ||
| mergeAttributesAndFlags(NewCI, *CI); | ||
| return Dst; | ||
| } | ||
|
|
@@ -819,13 +830,10 @@ Value *LibCallSimplifier::optimizeStrLCpy(CallInst *CI, IRBuilderBase &B) { | |
| return ConstantInt::get(CI->getType(), 0); | ||
| } | ||
|
|
||
| // Transform strlcpy(D, S, N) to memcpy(D, S, N') where N' is the lower | ||
| // bound on strlen(S) + 1 and N, optionally followed by a nul store to | ||
| // D[N' - 1] if necessary. | ||
| CallInst *NewCI = B.CreateMemCpy(Dst, Align(1), Src, Align(1), NBytes); | ||
| mergeAttributesAndFlags(NewCI, *CI); | ||
|
|
||
| if (!NulTerm) { | ||
|
|
@@ -844,7 +852,6 @@ Value *LibCallSimplifier::optimizeStrLCpy(CallInst *CI, IRBuilderBase &B) { | |
| // otherwise. | ||
| Value *LibCallSimplifier::optimizeStringNCpy(CallInst *CI, bool RetEnd, | ||
| IRBuilderBase &B) { | ||
| Value *Dst = CI->getArgOperand(0); | ||
| Value *Src = CI->getArgOperand(1); | ||
| Value *Size = CI->getArgOperand(2); | ||
|
|
@@ -921,11 +928,9 @@ Value *LibCallSimplifier::optimizeStringNCpy(CallInst *CI, bool RetEnd, | |
| /*M=*/nullptr, /*AddNull=*/false); | ||
| } | ||
|
|
||
| // st{p,r}ncpy(D, S, N) -> memcpy(align 1 D, align 1 S, N) when both | ||
| // S and N are constant. | ||
| CallInst *NewCI = B.CreateMemCpy(Dst, Align(1), Src, Align(1), N); | ||
| mergeAttributesAndFlags(NewCI, *CI); | ||
| if (!RetEnd) | ||
| return Dst; | ||
|
|
@@ -3357,7 +3362,9 @@ Value *LibCallSimplifier::optimizePrintFString(CallInst *CI, IRBuilderBase &B) { | |
| // Create a string literal with no \n on it. We expect the constant merge | ||
| // pass to be run after this pass, to merge duplicate strings. | ||
| FormatStr = FormatStr.drop_back(); | ||
| Value *GV = B.CreateGlobalString( | ||
| FormatStr, "str", | ||
| CI->getArgOperand(0)->getType()->getPointerAddressSpace()); | ||
|
There was a problem hiding this comment. If you're going to touch CreateGlobalString, please fix all the calls in this file at the same time. On targets with multiple address-spaces, is it actually legal to create a global like this? I thought the usual convention was to create a global in some specific address-space, then addrspacecast it to the correct address-space. There was a problem hiding this comment. Generally should try to use a context address space. For a newly synthesized global, there's DL.getDefaultGlobalsAddressSpace There was a problem hiding this comment. In this specific line we're replacing one global string with another, so it makes sense to use the original address space. I'll look into other uses within this file. There was a problem hiding this comment. getConstantStringInfo looks through addrspacecasts, so There was a problem hiding this comment. Alright, let's just use the default globals address space. I'm going to split this component into a separate PR. There was a problem hiding this comment. Split off into #118729 Writing the test for this exposed more places where we were failing to propagate address spaces correctly, which are addressed in that PR as well. |
||
| return copyFlags(*CI, emitPutS(GV, B, TLI)); | ||
| } | ||
|
|
||
|
|
@@ -3432,10 +3439,8 @@ Value *LibCallSimplifier::optimizeSPrintFString(CallInst *CI, | |
| return nullptr; // we found a format specifier, bail out. | ||
|
|
||
| // sprintf(str, fmt) -> llvm.memcpy(align 1 str, align 1 fmt, strlen(fmt)+1) | ||
| B.CreateMemCpy(Dest, Align(1), CI->getArgOperand(1), Align(1), | ||
| FormatStr.size() + 1); // Copy the null byte. | ||
| return ConstantInt::get(CI->getType(), FormatStr.size()); | ||
| } | ||
|
|
||
|
|
@@ -3470,9 +3475,7 @@ Value *LibCallSimplifier::optimizeSPrintFString(CallInst *CI, | |
|
|
||
| uint64_t SrcLen = GetStringLength(CI->getArgOperand(2)); | ||
| if (SrcLen) { | ||
| B.CreateMemCpy(Dest, Align(1), CI->getArgOperand(2), Align(1), SrcLen); | ||
| // Returns total number of characters written without null-character. | ||
| return ConstantInt::get(CI->getType(), SrcLen - 1); | ||
| } else if (Value *V = emitStpCpy(Dest, CI->getArgOperand(2), B, TLI)) { | ||
|
|
@@ -3570,11 +3573,7 @@ Value *LibCallSimplifier::emitSnPrintfMemCpy(CallInst *CI, Value *StrArg, | |
| Value *DstArg = CI->getArgOperand(0); | ||
| if (NCopy && StrArg) | ||
| // Transform the call to lvm.memcpy(dst, fmt, N). | ||
| copyFlags(*CI, B.CreateMemCpy(DstArg, Align(1), StrArg, Align(1), NCopy)); | ||
|
|
||
| if (N > Str.size()) | ||
| // Return early when the whole format string, including the final nul, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the memcmp case should probably have a similar wrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to change this, please change it to use the correct value -- which is TLI::getSizeTSize().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately,
TLI::getSizeTSize()contains a nice comment explaining that it always usesaddrspace(0), and noting that maybe it should consider alternatives, which is exactly the goal here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a few alternatives in here, none of them great:
IRBuilder::CreateMemCpyso that it can pick the right size. This requires updating a number of other passes throughout the tree, many of which don't depends on TargetLibraryInfo today.Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason getSizeTSize() unconditionally queries addrspace(0) is just that there aren't any in-tree targets where it needs to return anything else. Fixes welcome.
Passing in a integer to emitMemCmp where the size isn't getSizeTSize() just miscompiles, as far as I know. (CreateMemCpy works fine in any case because it's an intrinsic.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't think fixing
getSizeTSize()makes sense. It's just a wrapper aroundDataLayout::getPointerSizeInBits, which is already more widely accessible throughout the middle-end thanTargetLibraryInfois.If we really want the convenience name, then I think we should move
getSizeTSizetoDataLayout, and add an addrspace argument such that it is a pure wrapper aroundgetPointerSizeInBits.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSizeTSize() is supposed to be the bitwidth of the type size_t. There's only one size_t. This is useful because we're talking about C library functions, which are defined in terms of size_t.
If you want to query information about a specific address-space, we have appropriate datalayout APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be amenable to moving
getSizeTSize()ontoDataLayout? That would make it much easier to update all callers ofCreateMemCpy.I'll look at ensuring
getSizeTSize()is correct for CHERI targets in a separate PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think getSizeTSize belongs on the data layout. It's a property related to the C library, rather than the target itself.
For CHERI's purposes, shouldn't it be sufficient to consistently make use of getSizeTSize() in this file and patch getSizeTSize() to return the correct value? Maybe just making the default value be the index type size instead of the pointer type size would be sufficient?
The BuildLibCalls code itself already uses the correct types, we're just not passing in matching types here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've split such a change into #118747
There seems to be at least one test that concretely tests for the opposite behavior today, so we'll need to sort that out. If we can make that change, then most of the changes in this PR become irrelevant.