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

Sema::checkPointerTypesForAssignment calls IsFunctionConversion with From/To reversed #85415

Open
dougsonos opened this issue Mar 15, 2024 · 3 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@dougsonos
Copy link
Contributor

At the end of Sema::checkPointerTypesForAssignment:

  if (!S.getLangOpts().CPlusPlus &&
      S.IsFunctionConversion(ltrans, rtrans, ltrans))

ltrans and rtrans are the left and right sides of the assignment, and are therefore the destination and source types, respectively.

But:

bool Sema::IsFunctionConversion(QualType FromType, QualType ToType,
                                QualType &ResultTy) {

Here the source is first and the destination is second.

Maybe this a bug that has gone unnoticed because IsFunctionConversion tends to work symmetrically? (speculating)

Or maybe there is a good reason for checkPointerTypesForAssignment to invert the parameters, but it's not commented or at all apparent by source inspection.

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Mar 15, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 15, 2024

@llvm/issue-subscribers-clang-frontend

Author: Doug Wyatt (dougsonos)

At the end of `Sema::checkPointerTypesForAssignment`: ``` if (!S.getLangOpts().CPlusPlus && S.IsFunctionConversion(ltrans, rtrans, ltrans)) ```

ltrans and rtrans are the left and right sides of the assignment, and are therefore the destination and source types, respectively.

But:

bool Sema::IsFunctionConversion(QualType FromType, QualType ToType,
                                QualType &ResultTy) {

Here the source is first and the destination is second.

Maybe this a bug that has gone unnoticed because IsFunctionConversion tends to work symmetrically? (speculating)

Or maybe there is a good reason for checkPointerTypesForAssignment to invert the parameters, but it's not commented or at all apparent by source inspection.

@Sirraide
Copy link
Member

It’s also worth noting that, right below that, we also inconsistently pass the LHS and RHS as the FromType and ToType of other functions (comments added):

  if (IsInvalidCmseNSCallConversion(S, /*FromType=*/ltrans, /*ToType=*/rtrans))
  if (S.IsInvalidSMECallConversion(    /*FromType=*/rtrans, /*ToType=*/ltrans))

dougsonos pushed a commit to dougsonos/llvm-project that referenced this issue Mar 15, 2024
…ion() not change the type for C. Add comment about llvm#85415.
@shafik
Copy link
Collaborator

shafik commented Mar 15, 2024

So those lines look like there were touched by:

@sdesmalen-arm via 28b5f30
@momchil-velikov via 080d046
@zygoloid via 3c4f8d2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

No branches or pull requests

5 participants