Page MenuHomePhabricator

Warn when NumParams overflows
Needs ReviewPublic

Authored by Mordante on Jul 16 2019, 10:59 AM.

Details

Reviewers
rsmith
rjmccall
Summary

Before when the overflow occurred an assertion was triggered. Now check whether the maximum has been reached and warn properly.

This patch fixes bug 33162 which is marked as 'duplicate' of bug 19607. The original part of bug 19607 is fixed by D63975.

Diff Detail

Event Timeline

Mordante created this revision.Jul 16 2019, 10:59 AM
rjmccall added inline comments.Jul 26 2019, 11:29 AM
clang/include/clang/AST/Type.h
1523

This should probably go in FunctionTypeBitfields.

clang/lib/Parse/ParseDecl.cpp
6676

We don't usually cite bugs like this in the main source code except as "there's a bug with this, here's where we tracking fixing it". The comment explains the purpose of the diagnostic well enough (but please include a period).

Can you just drop the excess arguments here instead of cutting off parsing?

rjmccall added inline comments.Jul 26 2019, 11:56 AM
clang/lib/Parse/ParseDecl.cpp
6676

Or actually — let's move this out of the parser entirely, because (unlike prototype scope depth) we actually need to diagnose it in Sema as well because of variadic templates. Sema::BuildFunctionType seems like the right place.

Also, can you static_assert that function declarations support at least as many parameter declarations as we allow in types?

Mordante marked an inline comment as done.Jul 31 2019, 12:39 PM
Mordante added inline comments.
clang/lib/Parse/ParseDecl.cpp
6676

I tried your suggestion to move the test to Sema::BuildFunctionType but that function is only used for templates so it doesn't show the warning for other cases. Also the error generated will also print the note_ovl_candidate_substitution_failure diagnostic making the output rather unreadable. So I'll look for a better way to handle the variadic template case.

rjmccall added inline comments.Aug 1 2019, 8:05 AM
clang/lib/Parse/ParseDecl.cpp
6676

Hmm, yes, we should find a way to suppress follow-on errors like that. But we do need to be able to avoid problems even when building parameter lists that don't correspond to a declared function, e.g. with template <class... T> void foo(void (*fp)(T...));. (You'll need to pass the template arguments explicitly here, since we obviously can't infer an excessive number of parameters from a function whose parameter count is limited by the same constraint.)

And I always forget that function type parsing doesn't go through that function, sorry.

Mordante marked an inline comment as done.Aug 15 2019, 10:11 AM
Mordante updated this revision to Diff 215425.Aug 15 2019, 10:17 AM

Moved the testing from Parse to Sema.
Added additional safeguards for template instantiation.
Added more unit tests.
The comments may be a bit noisy, but they explain why the templates need to be tested at two locations.

rjmccall added inline comments.Aug 26 2019, 9:43 PM
clang/lib/Parse/ParseDecl.cpp
6233

The re-indentation here is wrong.

clang/lib/Sema/SemaDeclCXX.cpp
304

This is off by one: if the maximum number of parameters is 2, you want the diagnostic to appear on the parameter at index 2, not the parameter at index 1.

clang/lib/Sema/SemaOverload.cpp
6794

Unfortunately, you can't do this: a call can have more arguments than parameters because of variadic arguments. We probably *also* need to enforce an implementation limit on arguments, but it's not obvious that it should be the same limit.