[clang] assume_aligned incorrectly diagnoses a dependent return type #111573

AmrDeveloper · 2024-10-08T18:14:23Z

Fix assume_aligned incorrectly diagnoses a dependent return type

Fixes: #111563

llvmbot · 2024-10-08T18:14:52Z

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

Fix assume_aligned incorrectly diagnoses a dependent return type

Fixes: #111563

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

2 Files Affected:

(modified) clang/lib/Sema/SemaDeclAttr.cpp (+2)
(modified) clang/test/SemaCXX/builtin-assume-aligned-tmpl.cpp (+3)

diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index af983349a89b58..c1d1ef31db6e92 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1217,6 +1217,8 @@ static void handlePreferredName(Sema &S, Decl *D, const ParsedAttr &AL) {
 
 bool Sema::isValidPointerAttrType(QualType T, bool RefOkay) {
   if (RefOkay) {
+    if (T->isDependentType())
+      return true;
     if (T->isReferenceType())
       return true;
   } else {
diff --git a/clang/test/SemaCXX/builtin-assume-aligned-tmpl.cpp b/clang/test/SemaCXX/builtin-assume-aligned-tmpl.cpp
index 61b85557d6b294..4459580cdccc5d 100644
--- a/clang/test/SemaCXX/builtin-assume-aligned-tmpl.cpp
+++ b/clang/test/SemaCXX/builtin-assume-aligned-tmpl.cpp
@@ -52,6 +52,9 @@ T *atest3() __attribute__((assume_aligned(31, o))); // expected-error {{requeste
 template <typename T, int o>
 T *atest4() __attribute__((assume_aligned(32, o)));
 
+template<typename T>
+T atest5(int) __attribute__((assume_aligned(2)));
+
 void test22() {
   atest3<int, 5>();
   atest4<int, 5>();

erichkeane · 2024-10-08T18:30:38Z

clang/test/SemaCXX/builtin-assume-aligned-tmpl.cpp

 T *atest4() __attribute__((assume_aligned(32, o)));

+template<typename T>
+T atest5(int) __attribute__((assume_aligned(2)));


This change causes a number of attributes to change their behavior (see all uses of isValidPointerAttrType), so we should test each of those.

Additionally, we should separately check (via a different declaration) that the diagnostic fires when instantiated.

erichkeane · 2024-10-08T18:31:14Z

clang/lib/Sema/SemaDeclAttr.cpp


 bool Sema::isValidPointerAttrType(QualType T, bool RefOkay) {
  if (RefOkay) {
+    if (T->isDependentType())


This doesn't seem like the right place for this, we should be skipping ANY time the type is dependent, not only when a ref argument is acceptable.

Think you, updated

erichkeane · 2024-10-08T18:38:20Z

clang/lib/Sema/SemaDeclAttr.cpp

Probably valuable to audit the other uses of this function well, and remove checks for dependence there as redundant. And probably ensure the documentation of this function mentions that it skips on dependence.

Done, please check the updated documentation of the function

erichkeane · 2024-10-08T19:23:54Z

clang/include/clang/Sema/Sema.h

-  /// attributes. By default, we look through references (the behavior used by
-  /// nonnull), but if the second parameter is true, then we treat a reference
-  /// type as valid.
+  /// attributes. By default, we skip dependence and look through references


by default implies that the dependence check can bie disabled, but it very much cannot.

Thank you, all comments addressed

AmrDeveloper · 2024-10-09T16:29:30Z

This part in ObjectiveC tests conflict with the current solution, any suggestion how we can go around it right now until to be handled later

llvm-project/clang/test/SemaObjCXX/noescape.mm

Lines 202 to 205 in 18952bd

    
           // FIXME: clang shouldn't reject this. 
        
           noescapeFunc5(^{ 
        
             (void)b9; 
        
           });

@erichkeane @AaronBallman

erichkeane · 2024-10-09T16:29:44Z

clang/include/clang/Sema/Sema.h

-  /// attributes. By default, we look through references (the behavior used by
-  /// nonnull), but if the second parameter is true, then we treat a reference
-  /// type as valid.
+  /// attributes. We skip dependence By default, we look through references


again, we're not skipping dependence by default, we're ALWAYS skipping dependence. We skip references (presumably by default). I think the dependence behavior just needs its own sentence here.

Yes, i think i was planning to make it like We skip dependence and By default ..., or We skip dependence then By default ...

erichkeane · 2024-10-09T16:32:02Z

This part in ObjectiveC tests conflict with the current solution, any suggestion how we can go around it right now until to be handled later

llvm-project/clang/test/SemaObjCXX/noescape.mm

Lines 202 to 205 in 18952bd

// FIXME: clang shouldn't reject this.

noescapeFunc5(^{

(void)b9;

});

@erichkeane @AaronBallman

Doesn't the fixme above that say it was diagnosing unintentionally? If so, that fixme likely needs to be removed, and this is 'correct' now.

AmrDeveloper · 2024-10-09T16:34:51Z

This part in ObjectiveC tests conflict with the current solution, any suggestion how we can go around it right now until to be handled later

llvm-project/clang/test/SemaObjCXX/noescape.mm

Lines 202 to 205 in 18952bd

// FIXME: clang shouldn't reject this.

noescapeFunc5(^{

(void)b9;

});

@erichkeane @AaronBallman

Doesn't the fixme above that say it was diagnosing unintentionally? If so, that fixme likely needs to be removed, and this is 'correct' now.

Yes but still got call to deleted constructor of 'S6' and 'S6' has been explicitly marked deleted here errors on CI and local

AmrDeveloper · 2024-10-09T16:48:13Z

This part in ObjectiveC tests conflict with the current solution, any suggestion how we can go around it right now until to be handled later

llvm-project/clang/test/SemaObjCXX/noescape.mm

Lines 202 to 205 in 18952bd

// FIXME: clang shouldn't reject this.

noescapeFunc5(^{

(void)b9;

});

@erichkeane @AaronBallman

Doesn't the fixme above that say it was diagnosing unintentionally? If so, that fixme likely needs to be removed, and this is 'correct' now.

I handled it, i didn't know that // expected-note n is the number of expected notes,

Thank you

erichkeane · 2024-10-09T20:33:51Z

clang/include/clang/Sema/Sema.h

-  /// attributes. By default, we look through references (the behavior used by
-  /// nonnull), but if the second parameter is true, then we treat a reference
-  /// type as valid.
+  /// attributes. We skip dependence then By default, we look through references


Suggested change

/// attributes. We skip dependence then By default, we look through references

/// attributes. Dependent types are considered valid, so they can be checked during instantiation time. By default, we look through references

^^ make sure to clang-format.

erichkeane · 2024-10-09T20:38:27Z

clang/include/clang/Sema/Sema.h

Suggested change

/// treat a reference type as valid..

/// treat a reference type as valid.

Missed this!

erichkeane · 2024-10-09T20:38:34Z

clang/include/clang/Sema/Sema.h

Suggested change

/// attributes. Dependent types are considered valid, so they can be checked

/// attributes. Dependent types are considered valid so they can be checked

…lvm#111573) Fix `assume_aligned` incorrectly diagnoses a dependent return type Fixes: llvm#111563

AmrDeveloper added the clang Clang issues not falling into any other category label Oct 8, 2024

AmrDeveloper requested a review from AaronBallman October 8, 2024 18:14

llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Oct 8, 2024

erichkeane requested changes Oct 8, 2024

View reviewed changes

AmrDeveloper force-pushed the clang_assume_aligned_dependent_ret_type branch from fd3247e to 076c867 Compare October 8, 2024 18:34

erichkeane reviewed Oct 8, 2024

View reviewed changes

AmrDeveloper force-pushed the clang_assume_aligned_dependent_ret_type branch from 076c867 to 4202153 Compare October 8, 2024 18:52

AmrDeveloper requested a review from Endilll as a code owner October 8, 2024 18:52

erichkeane reviewed Oct 8, 2024

View reviewed changes

AmrDeveloper force-pushed the clang_assume_aligned_dependent_ret_type branch 2 times, most recently from dea97ae to 036f765 Compare October 8, 2024 20:51

AmrDeveloper mentioned this pull request Oct 9, 2024

[DAG] SDPatternMatch - match integer min/max patterns as well as the existing ISD nodes #108218

Closed

erichkeane reviewed Oct 9, 2024

View reviewed changes

AmrDeveloper force-pushed the clang_assume_aligned_dependent_ret_type branch 2 times, most recently from 3e83fd9 to d3f8538 Compare October 9, 2024 16:47

AmrDeveloper requested a review from erichkeane October 9, 2024 19:33

erichkeane reviewed Oct 9, 2024

View reviewed changes

AmrDeveloper force-pushed the clang_assume_aligned_dependent_ret_type branch from d3f8538 to a0066f7 Compare October 9, 2024 20:36

erichkeane approved these changes Oct 9, 2024

View reviewed changes

erichkeane requested changes Oct 9, 2024

View reviewed changes

[clang] assume_aligned incorrectly diagnoses a dependent return type

0f1405e

AmrDeveloper force-pushed the clang_assume_aligned_dependent_ret_type branch from a0066f7 to 0f1405e Compare October 9, 2024 20:41

erichkeane approved these changes Oct 9, 2024

View reviewed changes

AmrDeveloper merged commit 562999a into llvm:main Oct 10, 2024
8 checks passed

DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024

[clang] assume_aligned incorrectly diagnoses a dependent return type (l…

5086e26

…lvm#111573) Fix `assume_aligned` incorrectly diagnoses a dependent return type Fixes: llvm#111563

	/// attributes. We skip dependence then By default, we look through references
	/// attributes. Dependent types are considered valid, so they can be checked during instantiation time. By default, we look through references

	/// treat a reference type as valid..
	/// treat a reference type as valid.

[clang] assume_aligned incorrectly diagnoses a dependent return type #111573

[clang] assume_aligned incorrectly diagnoses a dependent return type #111573

Uh oh!

Conversation

AmrDeveloper commented Oct 8, 2024

Uh oh!

llvmbot commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper commented Oct 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Oct 9, 2024

Uh oh!

AmrDeveloper commented Oct 9, 2024

Uh oh!

AmrDeveloper commented Oct 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AmrDeveloper Oct 9, 2024 •

edited

Loading