Jordan- I've made your suggested code changes and have added a number of tests. As you mentioned, not all of the test work right but adding them in should give us a starting place for the future.
Zach On Tue, Mar 18, 2014 at 10:13 PM, Jordan Rose <[email protected]> wrote: > This looks good! Can you add more tests involving the new fix-it? Some ideas: > > - fixed-length array with the wrong character type > - fixed-length array with the right character type, but the size is too big > (with a FIXME) > - variable-length array > - pointer parameter typed as an array > - pointer parameter typed as an array with a static bound ("int buffer[static > 10]"). Yes, that's a thing. (We don't have to support it specially here, but > it shouldn't crash.) > > A few more comments on the code: > > // If it's an enum, get its underlying type. > if (const EnumType *ETy = QT->getAs<EnumType>()) > QT = ETy->getDecl()->getIntegerType(); > > This isn't your change, but I think that's supposed to be PT, not QT! > > + if(const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(RawQT)) { > > Please add a space before the open paren, now that it fits. > > Thanks again for working on this! It does seem much easier with both types > available. > Jordan > > > On Mar 18, 2014, at 17:03 , Zach Davis <[email protected]> wrote: > >> I think passing both types is a good way to go, it would keep it more >> generic than just passing the whole Expr. >> >> Attached is the modified patch. >> >> On Mon, Mar 10, 2014 at 11:48 AM, Jordan Rose <[email protected]> wrote: >>> I spent several minutes looking around for the appropriate function to call >>> here. getAdjustedParameterType() seems to do what we want, but it's >>> definitely meant for use at the declaration site, not the call site. I'm >>> thinking it's probably best to just pass both types, and only check the >>> original type for things like this. What do you think? >>> >>> Jordan >>> >>> >>> On Mar 7, 2014, at 8:58 , Zach Davis <[email protected]> wrote: >>> >>> Jordan- >>> >>> getDecayedType() faults when the QualType passed to it is a "size_t >>> *identifier" type. The only other place getDecayedType is called it >>> goes like: >>> >>> if (T->isArrayType() || T->isFunctionType()) >>> return getDecayedType(T) >>> >>> What we need is some way to re-decay the QualType to a valid >>> "function-parm-type" (which I assume happens somewhere?) >>> >>> I agree now that getCanonicalParmType is overkill. Maybe we just need >>> to do some checks so we can do the right decay based on the QualType? >>> >>> Zach >>> >>> On Wed, Mar 5, 2014 at 4:09 PM, Jordan Rose <[email protected]> wrote: >>> >>> You're asking for the canonical type, which is looking through typedefs. I >>> don't think that's what we want to do. What sort of segfaults did you get >>> when using the decayed type? >>> >>> Jordan >>> >>> >>> On Mar 5, 2014, at 13:19 , Zach Davis <[email protected]> wrote: >>> >>> Thanks for the comments. >>> >>> I have made the style changes and re-arranged the checks so wide >>> char types are also included. >>> >>> However, when I changed >>> >>> + QualType QT = Ctx.getCanonicalParamType(ArgQT) >>> >>> to getDecayedType, I got a number of seg faults. So I have left >>> it for now as GetCanonicalParamType. Along this same line, the >>> patch does cause an existing fixit test to fail, namely: >>> >>> size_t size_t_var; >>> scanf("%f", size_t_var); >>> ^- >>> %ul >>> >>> The test expects "%zu", not "%ul". This seems to be related to >>> the type of decay we do, but I don't know where to go from there. >>> >>> Zach >>> >>> On Wed, Mar 5, 2014 at 11:50 AM, Jordan Rose <[email protected]> wrote: >>> >>> Good idea! I have a few comments on the patch itself, but the general >>> implementation seems sound. >>> >>> + QualType QT = Ctx.getCanonicalParamType(ArgQT); >>> >>> This probably doesn't need to be canonical; just "getDecayedType" should be >>> good enough. >>> >>> >>> + else { >>> + // if we can determine the array length, >>> + // we should include it as a field width >>> + const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(ArgQT); >>> + if (CAT && CAT->getSizeModifier() == ArrayType::Normal) >>> >>> Three notes: >>> >>> - This would more idiomatically be spelled "else if (const ConstantArrayType >>> *CAT = ...)". >>> - Please use complete sentences and punctuation for comments in the LLVM >>> code. >>> - Is there any reason the same logic won't work with wide string buffers? >>> >>> >>> + bool success = fixedFS.fixType(Ex->IgnoreImpCasts()->getType(), >>> S.getLangOpts(), >>> >>> Please reflow the arguments to stay within 80 columns. >>> >>> Thanks for working on this! >>> Jordan >>> >>> >>> On Mar 4, 2014, at 21:07 , Zach Davis <[email protected]> wrote: >>> >>> Background: Bug 18412 suggests that the compiler should issue a >>> security warning when a scanf %s format specifier does not include a >>> field width. This is the second of 3 patches working toward this >>> (first was r202114). >>> >>> This patch updates the fixit system to suggest a field width for %s >>> specifiers when the length of the target array is a know fixed size. >>> >>> Example: >>> >>> char a[10]; >>> scanf("%s", a); >>> ^- >>> %9s >>> >>> In order to determine the array length, the fixType function needs to >>> know the complete type of the argument, otherwise it is just the raw >>> pointer type that we can't reason about. >>> <scanf_fixit.patch>_______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >>> <scanf_fixit2.patch> >>> >>> >>> >> <scanf_fixit_4.patch> >
scanf_fixit5.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
