Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] Fix an integer overflow issue in computing CTAD's parameter depth #128704

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Feb 25, 2025

There were some cases where we computed incorrect template parameter depths for synthesized CTAD, invalid as they might be, we still shouldn't crash anyway.

Technically the only scenario in which the inner function template's depth is 0 is when it lives within an explicit template specialization, where the template parameter list is empty.

Fixes #128691

@zyn0217 zyn0217 requested review from cor3ntin and hokein February 25, 2025 11:59
Copy link
Collaborator

@hokein hokein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix!

NamedDecl *NewParam = transformTemplateParameter(
SemaRef, DC, Param, Args, Index + Depth1IndexAdjustment, Depth - 1);
SemaRef, DC, Param, Args, Index + Depth1IndexAdjustment,
Depth ? Depth - 1 : 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, for fully explicit specializations where the template parameter list is empty <>, they don't contribute to the depth. This makes me think we should use Depth directly if FTD->getDeclContext() is an explicit specialization?

Would it be possible the Depth is not 0 in that case? an explicit template specialization is inside a template class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm interesting question. Afaict, the transform ConvertConstructorToDeductionGuideTransform only handles class templates nested no more than one level. So basically:

  • For one-level nested cases (e.g., https://godbolt.org/z/x94dzMaeo), the original template parameter depth is 1, so the new depth would be definitely 0, which seems correct.
  • For multi-level nested cases, we synthesize a CTAD guide with all templates except the last level being instantiated, effectively reducing it to the one-level scenario.

So I think that, in the end the depth should always be 0. But I'm happy to use Depth instead of a literal 0 if that seems more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I realized we sometimes still want to use Depth - 1 even if it is an explicit specialization (because it could be nested as in the first case, and the transformed depth should be 0) - so I think this is good enough?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think that, in the end the depth should always be 0. But I'm happy to use Depth instead of a literal 0 if that seems more appropriate.

I agree, this seems like always the case, Depth for the new transformed template parameter is 0.

I think we need some comments for the special 0 case.

@hokein
Copy link
Collaborator

hokein commented Feb 25, 2025

(We should have been doing this wrong prior to 20, but it didn't crash because it called Decl::getTemplateDepth(), which returns 0 for the transformed template parameter declaration)

I think you're right. The depth is already incorrect in clang19 etc. https://godbolt.org/z/d9xhxh3EM

 |-TemplateTypeParmDecl 0x175b5b88 <line:6:13, col:22> col:22 typename depth 32767 index 1 T

Copy link
Collaborator

@hokein hokein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a release note as it fixes an existing issue.

NamedDecl *NewParam = transformTemplateParameter(
SemaRef, DC, Param, Args, Index + Depth1IndexAdjustment, Depth - 1);
SemaRef, DC, Param, Args, Index + Depth1IndexAdjustment,
Depth ? Depth - 1 : 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think that, in the end the depth should always be 0. But I'm happy to use Depth instead of a literal 0 if that seems more appropriate.

I agree, this seems like always the case, Depth for the new transformed template parameter is 0.

I think we need some comments for the special 0 case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants