[PATCH] D147037: [Clang][ICE] Corrected invalid invalid parameter index on some attributes with invalid indices applied to varargs functions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3764-3767
   if (!checkFunctionOrMethodParameterIndex(S, D, AL, 1, IdxExpr, Idx))
     return;
-
+  if (Idx.getASTIndex() >= getFunctionOrMethodNumParams(D))
+    return;
----------------
tbaeder wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > Did you look into fixing this within `checkFunctionOrMethodParameterIndex()` instead? That way, all callers of the API get the correct behavior instead of having to find individual attributes to check the logic (I believe there are other attributes with the same underlying problem).
> > I would also expect any such issue to diagnose.  Also, isn't there an off-by-one error here in the case of variadic args?
> `handleFormatAttr()` has:
> ```
>   if (Idx < 1 || Idx > NumArgs) {
>     S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds)
>         << AL << 2 << IdxExpr->getSourceRange();
>     return;
>   }
> ```
> 
> which is also what I'd expect to see here.
> 
> 
I agree w/ @aaron.ballman here we should handle this in `checkFunctionOrMethodParameterIndex` it is used in many places and AFAICT they all have the same issue, none diagnose this case but paper it over.

There is definitely a bigger clean-up here but we can do a targeted fix now and remove a crash bug and leave the larger fixing for a follow-up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147037/new/

https://reviews.llvm.org/D147037