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 remove{CVR|Fast}Qualifiers with 64-bit Qualifiers::Mask #90329

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

kovdan01
Copy link
Contributor

After #84384, Qualifiers::Mask becomes 64-bit. So, operations like Mask &= ~U32 where U32 is unsigned produce undesirable results since higher 32 bits of Mask become zeroed while they should be preserved. Fix that by explicitly casting unsigned values to uint64_t in such operations. Signatures of fixed functions are intentionally left intact instead of changing the argument itself to uint64_t to keep things consistent with other functions working with the same qualifiers and to emphasize that 64-bit masks should not be used for these types of qualifiers.

After llvm#84384, `Qualifiers::Mask` becomes 64-bit. So, operations like
`Mask &= ~U32` where `U32` is `unsigned` produce undesirable results
since higher 32 bits of `Mask` become zeroed while they should be
preserved. Fix that by explicitly casting `unsigned` values to `uint64_t`
in such operations. Signatures of fixed functions are intentionally left
intact instead of changing the argument itself to `uint64_t` to keep things
consistent with other functions working with the same qualifiers and to
emphasize that 64-bit masks should not be used for these types of
qualifiers.
@kovdan01 kovdan01 marked this pull request as ready for review April 27, 2024 08:47
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 27, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 27, 2024

@llvm/pr-subscribers-clang

Author: Daniil Kovalev (kovdan01)

Changes

After #84384, Qualifiers::Mask becomes 64-bit. So, operations like Mask &= ~U32 where U32 is unsigned produce undesirable results since higher 32 bits of Mask become zeroed while they should be preserved. Fix that by explicitly casting unsigned values to uint64_t in such operations. Signatures of fixed functions are intentionally left intact instead of changing the argument itself to uint64_t to keep things consistent with other functions working with the same qualifiers and to emphasize that 64-bit masks should not be used for these types of qualifiers.


Full diff: https://github.com/llvm/llvm-project/pull/90329.diff

1 Files Affected:

  • (modified) clang/include/clang/AST/Type.h (+2-2)
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index dff02d4861b3db..5c7d5396658b57 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -480,7 +480,7 @@ class Qualifiers {
   }
   void removeCVRQualifiers(unsigned mask) {
     assert(!(mask & ~CVRMask) && "bitmask contains non-CVR bits");
-    Mask &= ~mask;
+    Mask &= ~static_cast<uint64_t>(mask);
   }
   void removeCVRQualifiers() {
     removeCVRQualifiers(CVRMask);
@@ -609,7 +609,7 @@ class Qualifiers {
   }
   void removeFastQualifiers(unsigned mask) {
     assert(!(mask & ~FastMask) && "bitmask contains non-fast qualifier bits");
-    Mask &= ~mask;
+    Mask &= ~static_cast<uint64_t>(mask);
   }
   void removeFastQualifiers() {
     removeFastQualifiers(FastMask);

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

LGTM

@kovdan01
Copy link
Contributor Author

@AaronBallman Please let me know if you have some comments on this or if can be merged

@cor3ntin
Copy link
Contributor

@kovdan01 Aaron is not available this week. This looks sensible. Do you need me to merge it for you?

@kovdan01
Copy link
Contributor Author

@cor3ntin Thanks for feedback! I'll merge it myself

@kovdan01 kovdan01 merged commit 91f251c into llvm:main Apr 29, 2024
8 of 9 checks passed
kovdan01 added a commit that referenced this pull request Apr 30, 2024
…sions (#84387)

Depends on #84384 and #90329

This adds support for `DW_TAG_LLVM_ptrauth_type` entries corresponding
to explicitly signed types (e.g. free function pointers) in lldb user
expressions. Applies PR apple#8239
from Apple's downstream and also adds tests and related code.

---------

Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
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" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants