[PATCH] D45180: libcalls must check for "RtLibUseGOT" metadata during simplification
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 2 17:43:36 PDT 2018
efriedma added inline comments.
================
Comment at: lib/Transforms/Utils/BuildLibCalls.cpp:991
if (File->getType()->isPointerTy())
- inferLibFuncAttributes(*M->getFunction("fputc"), *TLI);
+ inferLibFuncAttributes(*M->getFunction("fputc"), *TLI, M);
Char = B.CreateIntCast(Char, B.getInt32Ty(), /*isSigned*/true,
----------------
tmsriram wrote:
> efriedma wrote:
> > tmsriram wrote:
> > > efriedma wrote:
> > > > The "if (File->getType()->isPointerTy())" here is suspicious... I think we always need to honor the RtLibUseGOT metadata?
> > > Good catch, thanks! Looking at the other functions only the F{PutS, Putc,Write} functions have this where it is safe to tag the param values with no capture. When would this condition File->getType()->isPointerTy() be false?
> > Never, I think? At least, I can't come up with a case where it would be false; all the SimplifyLibCalls transforms should call isValidProtoForLibFunc, which should check that FILE* parameters are pointers.
> >
> > That said, there is a related problem here; we don't ever check that the function returned by `M->getFunction("fputc")` has the expected signature, so this could crash if there's an existing function named fputc with the wrong signature. (Defining a function like that has undefined behavior, so it's unlikely to come up in practice, but we still shouldn't crash.)
> > Never, I think? At least, I can't come up with a case where it would be false; all the SimplifyLibCalls transforms should call isValidProtoForLibFunc, which should check that FILE* parameters are pointers.
>
> I will leave it as it is then. I could instead add just the "nonlazybind" attribute here but I could create another patch that removes the condition
>
>
>
>
> > That said, there is a related problem here; we don't ever check that the function returned by M->getFunction("fputc") has the expected signature, so this could crash if there's an existing function named fputc with the wrong signature. (Defining a function like that has undefined behavior, so it's unlikely to come up in practice, but we still shouldn't crash.)
> >
>
> This probably needs to be done in a separate patch too, I see we do M->getOrInsertFunction with type and also check if the libcall is indeed there in TLI. I see, so you want to overload fputs and that gets picked up, sigh.
>
Okay; I'm fine with fixing it in a separate patch.
https://reviews.llvm.org/D45180
More information about the llvm-commits
mailing list