[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D134475#4655362 , @RIscRIpt wrote:

> Should I re-submit it to GitHub?

While I hate that we'll lose all of the Phabriator history on this one, I don't 
see being able to complete this review in the next two weeks before Phab closes 
down.  The C++ Standards meeting is in 2 weeks, which makes it particularly 
difficult.

If you can properly rebase, submit this to github, and include a summary of the 
Phab discussion in a way that easy enough to catch back up on, I'll do a deep 
dive into this in the next week or 3.

Sorry for the delay, this fell off my radar somehow.




Comment at: clang/test/SemaCXX/ms-constexpr.cpp:30
+/*
+// TODO: Add support for [[msvc::constexpr]] constructor
+struct S2 {

I see this is still TODO, part of the reason I have been putting off review, I 
thought this patch wasn't ready for review.  Whats going on here?  When is this 
intended to be implemented?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134475/new/

https://reviews.llvm.org/D134475

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3701
+  QualType PointeeType = E->getType()->getPointeeType();
+  if (PointeeType.isNull())
+return false;

eddyz87 wrote:
> erichkeane wrote:
> > We override `operator bool` to make this work.
> Sorry, just to clarify, currently such modification fails with the following 
> error:
> 
> ```
> lang=c++
> clang/lib/CodeGen/CGExpr.cpp:3710:7: error: invalid argument type 'QualType' 
> to unary expression
>   if (!PointeeType)
>   ^~~~
> 1 error generated.
> ```
> 
> And you want me to modify `QualType` as follows:
> 
> ```
> --- a/clang/include/clang/AST/Type.h
> +++ b/clang/include/clang/AST/Type.h
> @@ -796,6 +796,8 @@ public:
>  return getTypePtr();
>}
>  
> +  explicit operator bool() const { return isNull(); }
> +
>bool isCanonical() const;
>bool isCanonicalAsParam() const;
> ```
> 
> Right?
No, don't do that, you can leave it just checking isNull.  I could have sworn 
we already had that operator, but perhaps it was removed at one point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

2 nits on the CFE, else LGTM!  Obviously you still need an LLVM reviewer for 
the backend changes.




Comment at: clang/lib/CodeGen/CGExpr.cpp:3701
+  QualType PointeeType = E->getType()->getPointeeType();
+  if (PointeeType.isNull())
+return false;

We override `operator bool` to make this work.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3703
+return false;
+  if (auto *BaseDecl = PointeeType->getAsRecordDecl())
+return hasBPFPreserveStaticOffset(BaseDecl);

if possible?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: llvm/test/CodeGen/BPF/preserve-static-offset/load-align.ll:61
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 18.0.0 (/home/eddy/work/llvm-project/clang 
c899a1ca75d0f1b559204eff79a2578d2cafc7ab)"}
+!2 = !{!3, !4, i64 128}

eddyz87 wrote:
> erichkeane wrote:
> > eddyz87 wrote:
> > > erichkeane wrote:
> > > > Are we sure we want to do something like this?  It seems this both 
> > > > depends on YOUR computer AND us never releasing Clang 18.
> > > Are you sure this would be an issue?
> > > The specific line is not a part of a CHECK and I tried the following 
> > > command using my system's llvm 16 opt:
> > > 
> > > ```
> > > opt -O2 -mtriple=bpf-pc-linux -S -o - load-align.ll
> > > ```
> > > 
> > > And module was loaded / processed w/o any issues.
> > > In general grepping shows that people don't usually mask these in tests:
> > > 
> > > ```
> > > $ cd llvm/test/CodeGen/
> > > $ ag '{!"clang version' | wc -l
> > > 452
> > > ```
> > I don't write LLVM tests ever, so I'm not sure.  It just seems odd to 
> > provide that much irrelevant info, perhaps one of hte LLVM reviewers can 
> > comment.  Also, look at those ~450 and see what they contain?
> > Also, look at those ~450 and see what they contain?
> Same random clang versions:
> 
> ```
> $ ag '{!"clang version' | head
> X86/debug-loclists.ll:129:!6 = !{!"clang version 10.0.0 (trunk 374581) 
> (llvm/trunk 374579)"}
> X86/dbg-combine.ll:84:!11 = !{!"clang version 3.7.0 (trunk 227074)"}
> X86/debuginfo-locations-dce.ll:46:!6 = !{!"clang version 8.0.0 (trunk 
> 339665)"}
> X86/pr31242.ll:48:!5 = !{!"clang version 4.0.0 (trunk 288844)"}
> X86/catchpad-regmask.ll:140:!1 = !{!"clang version 3.8.0 "}
> X86/debug-nodebug-crash.ll:48:!5 = !{!"clang version 4.0.0"}
> X86/limit-split-cost.mir:64:  !2 = !{!"clang version 7.0.0 (trunk 335057)"}
> X86/swap.ll:169:!1 = !{!"clang version 9.0.0 (trunk 352631) (llvm/trunk 
> 352632)"}
> X86/dwarf-aranges-available-externally.ll:65:!16 = !{!"clang version 15.0.0 
> (https://github.com/llvm/llvm-project.git 
> 2f52a868225755ebfa5242992d3a650ac6aadce7)"}
> X86/label-annotation.ll:96:!7 = !{!"clang version 9.0.0 
> (g...@github.com:llvm/llvm-project.git 
> 7f9a008a2db285aca57bfa0c09858c9527a7aa98)"}
> ```
It seems at least removing your home-path would be a good idea, but I can't 
really review these either.

Note the 'trunk #' is from our SVN days, so that isn't particularly useful 
at all.  IMO everything in the parens is worthless in the tests, but hopefully 
someone familiar with the LLVM tests can stop by and correct me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3700
+return false;
+  if (auto *BaseDecl = E->getType()->getPointeeType()->getAsRecordDecl())
+return hasBPFPreserveStaticOffset(BaseDecl);

eddyz87 wrote:
> erichkeane wrote:
> > getPointeeType can also return nullptr, so unless you have a test elsewhere 
> > to ensure it isn't, you likely have to do a little more work here (and if 
> > so, I'd need an assert).
> Is it? I actually double-checked this before pushing an update, clangd jumps 
> to the following definition:
> 
> ```
> lang=cpp
> QualType Type::getPointeeType() const {
>   if (const auto *PT = getAs())
> return PT->getPointeeType();
>   ...
>   return {};
> }
> ```
> 
> The `getAsRecordDecl()` can return null indeed, but that null is checked.
More correctly, it returns an empty `QualType`.  The `operator->` on that will 
cause a `nullptr`, causing the call to `Type::getAsRecordDecl` to have a 
`nullptr` `this`.



Comment at: llvm/test/CodeGen/BPF/preserve-static-offset/load-align.ll:61
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 18.0.0 (/home/eddy/work/llvm-project/clang 
c899a1ca75d0f1b559204eff79a2578d2cafc7ab)"}
+!2 = !{!3, !4, i64 128}

eddyz87 wrote:
> erichkeane wrote:
> > Are we sure we want to do something like this?  It seems this both depends 
> > on YOUR computer AND us never releasing Clang 18.
> Are you sure this would be an issue?
> The specific line is not a part of a CHECK and I tried the following command 
> using my system's llvm 16 opt:
> 
> ```
> opt -O2 -mtriple=bpf-pc-linux -S -o - load-align.ll
> ```
> 
> And module was loaded / processed w/o any issues.
> In general grepping shows that people don't usually mask these in tests:
> 
> ```
> $ cd llvm/test/CodeGen/
> $ ag '{!"clang version' | wc -l
> 452
> ```
I don't write LLVM tests ever, so I'm not sure.  It just seems odd to provide 
that much irrelevant info, perhaps one of hte LLVM reviewers can comment.  
Also, look at those ~450 and see what they contain?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D133361#4652102 , @eddyz87 wrote:

> Rebase, changes as requested by @aaron.ballman and @erichkeane.
>
> Hi @aaron.ballman, @erichkeane,
>
> Thank you for taking a look.
> I beleive this commit covers all feedback except "clang version"
> metadata comment by @erichkeane, I added inline reply there.
>
>> This will also need reviewers for the LLVM changes -- any ideas on
>> who usually reviews BPF-related changes in LLVM?
>
> I'll communicate with @ast and @yonghong-song.

I don't see the comment response you had to me.




Comment at: clang/lib/CodeGen/CGExpr.cpp:3700
+return false;
+  if (auto *BaseDecl = E->getType()->getPointeeType()->getAsRecordDecl())
+return hasBPFPreserveStaticOffset(BaseDecl);

getPointeeType can also return nullptr, so unless you have a test elsewhere to 
ensure it isn't, you likely have to do a little more work here (and if so, I'd 
need an assert).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Looks like there is quite a few places in the LLVM build that need updating 
(IteratorTest.cpp, Wasm.h, ElfDumper.cpp) so that builds don't fail as we 
commit this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

The CFE stuff is pretty innocuous and I don't see a reason to stop this on our 
accord, but would like someone familiar with the LLVM stuff to review this at 
one point.




Comment at: clang/lib/CodeGen/CGExpr.cpp:3694
+static bool hasBPFPreserveStaticOffset(const RecordDecl *D) {
+  return D->getAttr();
+}





Comment at: clang/lib/CodeGen/CGExpr.cpp:3698
+static bool hasBPFPreserveStaticOffset(const Expr *E) {
+  if (auto *PtrType = dyn_cast(E->getType()))
+if (auto *BaseDecl = PtrType->getPointeeType()->getAsRecordDecl())

You can use `E->getType()->getPointeeType()` here instead, unless you REALLY 
care that this is only a normal-pointer deref (and not a PMF or reference type).



Comment at: clang/lib/CodeGen/CGExpr.cpp:3778
 
+  if (Base && hasBPFPreserveStaticOffset(Base))
+addr = wrapWithBPFPreserveStaticOffset(CGF, addr);

I'd suggest just making `hasBPFPreserveStaticOffset` be `nullptr` tolerant and 
remove the 1st half of this.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7601
+  auto *Rec = cast(D);
+  Rec->addAttr(::new (S.Context) BPFPreserveStaticOffsetAttr(S.Context, AL));
+}

This should use `BPFPreserveStaticOffsetAttr::Create` instead of using 
placement `new`.  We've been meaning to translate these all at one point, but 
never got around to it.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8871
+  case ParsedAttr::AT_BPFPreserveStaticOffset:
+handleBPFPreserveStaticOffsetAttr(S, D, AL);
+break;

aaron.ballman wrote:
> 
Or this, this is probably better.



Comment at: llvm/test/CodeGen/BPF/preserve-static-offset/load-align.ll:61
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 18.0.0 (/home/eddy/work/llvm-project/clang 
c899a1ca75d0f1b559204eff79a2578d2cafc7ab)"}
+!2 = !{!3, !4, i64 128}

Are we sure we want to do something like this?  It seems this both depends on 
YOUR computer AND us never releasing Clang 18.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D147844#4651113 , @chaitanyav 
wrote:

> Bootstrapping build failed due to -werror flag 
> (https://buildkite.com/llvm-project/libcxx-ci/builds/30031)
>
>| 
> /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-4613821dc47f-1/llvm-project/libcxx-ci/libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp:43:18:
>  error: operator '?:' has lower precedence than '&'; '&' will be evaluated 
> first [-Werror,-Wbitwise-conditional-parentheses]
>   # |43 |   return val & 1 ? 1 : 0;
>   # |   |  ~~~ ^
>   
>   
>   # | 
> /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-4613821dc47f-1/llvm-project/libcxx-ci/libcxx/test/std/containers/unord/unord.multiset/eq.different_hash.pass.cpp:64:19:
>  error: operator '?:' has lower precedence than '&'; '&' will be evaluated 
> first [-Werror,-Wbitwise-conditional-parentheses]
>   # |64 |   return *val & 1 ? 1 : 0;
>   # |   |   ^

That looks like it is just in a test in libcxx, so at least that is a fairly 
simple thing to fix.  I'd suggest just adding the parens in there (and I'm sure 
that is something that @ldionne would have no problem with), or adding the -Wno 
flag to the tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D148381#4649683 , @void wrote:

> Friendly Ping.

Aaron is away at a conference this week, so hopefully he'll do another pass 
when he gets back next week.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148381/new/

https://reviews.llvm.org/D148381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157331: [clang] Implement C23

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D157331#4648417 , @pirama wrote:

> @aaron.ballman do you have additional comments or does this patch looks good 
> to merge?

Aaron is away this week at a conference in Norway (see his Discourse 
announcement), so you're not likely to see a response until next week.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157331/new/

https://reviews.llvm.org/D157331

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I've got no further comments/concerns.




Comment at: clang/include/clang/AST/Decl.h:4272-4275
+FieldDecl *FD = nullptr;
+for (FieldDecl *Field : fields())
+  FD = Field;
+return FD;

aaron.ballman wrote:
> void wrote:
> > aaron.ballman wrote:
> > > Could this be implemented as: `return !field_empty() ? 
> > > *std::prev(field_end()) : nullptr;` ? Then maybe someday we'll get better 
> > > iterator support that isn't forward-only and this code will automagically 
> > > get faster.
> > Using `std::prev` on a forward iterator won't work:
> > 
> > https://stackoverflow.com/questions/23868378/why-stdprev-does-not-fire-an-error-with-an-iterator-of-stdunordered-set
> > 
> > `std::prev` itself is defined only for bidirectional iterators:
> > 
> > ```
> >  template
> > _GLIBCXX_NODISCARD
> > inline _GLIBCXX17_CONSTEXPR _BidirectionalIterator
> > prev(_BidirectionalIterator __x, typename
> >  iterator_traits<_BidirectionalIterator>::difference_type __n = 1)
> > {
> > ...
> > ```
> > 
> Drat! Oh well, it was a lovely thought.
something like `!field_empty() ? *std::advance(field_begin(), 
std::distance(field_begin(), field_end() - 1) : nullptr`

would do what Aaron would like, though perhaps with an extra run through the 
fields list until then.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6371
+def err_flexible_array_counted_by_attr_refers_to_self : Error<
+  "counted_by field %0 cannot refer to the flexible array">;
+def err_flexible_array_counted_by_attr_field_not_integral : Error<

void wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > We try to wrap syntax elements in single quotes in our diagnostics so 
> > > it's more visually distinct
> > This one isn't clear... something more specifically about 'cannot point to 
> > itself' is perhaps more useful/explainatory.
> > 
> > Also, the beginning of all of these is really quite awkward as well, 
> > perhaps something like:
> > 
> > `field %0 referenced by 'counted_by' attribute ...` ?
> I reworded them a bit.
> 
> As for referring to itself, it could be a bit confusing to say that it 
> 'cannot point to itself' because the 'itself' in that is the attribute, not 
> the flexible array member. I think it's more-or-less clear what the error's 
> referring to. The user can use this attribute only with a flexible array 
> member. So they should know what what's meant by the message.
I think the reword helps, I am less confident that referring to the 'flexible 
array member' is clear. One of the things I've noticed in the past is users 
'trying' attributes without looking them up to see what they do.  Frankly, I 
think that should be a supported strategy, and one that we manage by making 
clear diagnostics.

I'm up for suggestions/hopeful someone else will come up with something better, 
but I can hold my nose at this if we don't have anything better (and in fact, 
my attempts to put a strawman up were at least as bad).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148381/new/

https://reviews.llvm.org/D148381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/AST/DeclBase.h:482
+  /// Whether it resembles a flexible array member.
+  static bool isFlexibleArrayMemberLike(
+  ASTContext , const Decl *D, QualType Ty,

Why isn't this a member function?



Comment at: clang/include/clang/Basic/Attr.td:4246
+private:
+  SourceRange countedByFieldLoc;
+public:

aaron.ballman wrote:
> Teeny tiniest of coding style nits :-D
Should we instead be capturing the field itself, rather than its location?  It 
seems to me that would be more useful?



Comment at: clang/include/clang/Basic/AttrDocs.td:7203
+same structure holding the count of elements in the flexible array. This
+information can be used to improve the results of array bound sanitizer and the
+``__builtin_dynamic_object_size``.

As this is the purpose of this attribute, it seems to me that the documentation 
should headline/more obviously highlight that the purpose here is to improve 
the result of the sanitizer when it comes to flexible array members.  



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6371
+def err_flexible_array_counted_by_attr_refers_to_self : Error<
+  "counted_by field %0 cannot refer to the flexible array">;
+def err_flexible_array_counted_by_attr_field_not_integral : Error<

aaron.ballman wrote:
> We try to wrap syntax elements in single quotes in our diagnostics so it's 
> more visually distinct
This one isn't clear... something more specifically about 'cannot point to 
itself' is perhaps more useful/explainatory.

Also, the beginning of all of these is really quite awkward as well, perhaps 
something like:

`field %0 referenced by 'counted_by' attribute ...` ?



Comment at: clang/lib/AST/DeclBase.cpp:482
+
+  RecordDecl::field_iterator FI(
+  DeclContext::decl_iterator(const_cast(FD)));

Whats going on here?  Is this simply an attempt to see if this is the last one? 
Can we combine this effort with the 'getLastFieldDecl' above?  Alternatively, 
can we get a good comment here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148381/new/

https://reviews.llvm.org/D148381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135341: adds `__reference_constructs_from_temporary`

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

All of @cor3ntin 's suggestions are really good ones, I'm ok with this once he 
is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135341/new/

https://reviews.llvm.org/D135341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135341: [clang] adds `__reference_constructs_from_temporary`

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Is the changes to cxx_status.html still relevant/current?  Also, did we ever 
figure out what GCC did for a builtin here?  Do they have one yet?  If not, 
have we encouraged them to implement this one?  If so, DID they implement this 
one?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135341/new/

https://reviews.llvm.org/D135341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159490: Fix warning in MSVC

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Fixed in 6c0b9e357617 
.  I 
didn't verify ahead of time, but hopefully the bots will yell at ME this time 
and I'll recognize it right away.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159490/new/

https://reviews.llvm.org/D159490

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159490: Fix warning in MSVC

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D159490#4643196 , 
@giulianobelinassi wrote:

> In D159490#4643191 , @fdeazeve 
> wrote:
>
>> I think this may have broken builds:
>>
>>   worktrees/src1/clang/lib/AST/DeclPrinter.cpp:293:48: error: function 
>> definition is not allowed here
>>   static bool mustPrintOnLeftSide(const Attr *A) {
>>
>> Probably missing a `}`?
>
> No, there is just an extra switch statement that somehow got into the patch. 
> I will fix this in a second.

Yikes, what I get for committing and not building first :)  I believe 
@giulianobelinassi still doesn't have commit access, so I'll just fix it as 
review-after-commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159490/new/

https://reviews.llvm.org/D159490

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159490: Fix warning in MSVC

[Erich Keane via Phabricator via cfe-commits]

erichkeane closed this revision.
erichkeane added a comment.

Forgot to close the phab review automatically, but: 
0323938d3c7a1a48b60a7dea8ec7300e98b4a931 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159490/new/

https://reviews.llvm.org/D159490

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159490: Fix warning in MSVC

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

This seems good enough to me.  I'll commit it for you as before.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159490/new/

https://reviews.llvm.org/D159490

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D159126#4642023 , @erichkeane 
wrote:

> In D159126#4642010 , @thakis wrote:
>
>> Looks like the test is missing a RUN line and hence makes lit fail: 
>> http://45.33.8.238/linux/117631/step_7.txt
>>
>> Please take a look and revert for now if it takes a while to fix.
>
> Won't take a while, it is a pretty simple change, I'll work on it now.

Should be fixed in 390b48675be80420f471bd3be74577495b1b1897 
.  We were 
ALSO missing a 'expected-no-diagnostics' line


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159126/new/

https://reviews.llvm.org/D159126

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D159126#4642010 , @thakis wrote:

> Looks like the test is missing a RUN line and hence makes lit fail: 
> http://45.33.8.238/linux/117631/step_7.txt
>
> Please take a look and revert for now if it takes a while to fix.

Won't take a while, it is a pretty simple change, I'll work on it now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159126/new/

https://reviews.llvm.org/D159126

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

I think this is alright.  Please see if you can figure out and fix what the 
pre-commit clang-format issue is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157526/new/

https://reviews.llvm.org/D157526

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:312
+  // printing on the left side for readbility.
+else if (const VarDecl *VD = dyn_cast(D);
+   VD && VD->getInit() &&

giulianobelinassi wrote:
> erichkeane wrote:
> > While compiling this, I discovered that the lack of curley braces on the 
> > `else if (canPrintOnLeftSide(A))` means that this `else-if` is actually an 
> > else to THAT instead, despite its indention (which implies it should be 
> > associated with the `if (const FunctionDecl...`).
> > 
> > Which is it?  I presume the indenting is what you mean, but it hasn't been 
> > tested in a way that matters.  Can you add a test that validates the 
> > var-decl printing on the LHS ONLY happens when it 'can' print on the LHS?
> Sorry for the warning there. I will post a fixed version of this as soon as 
> my build with `-Werror=all` completes.
> 
> Just to be clear, the intended code with braces is:
> ```
>   AttrPrintLoc AttrLoc = AttrPrintLoc::Right;
>   if (mustPrintOnLeftSide(A))
> // If we must always print on left side (e.g. declspec), then mark as
> // so.
> AttrLoc = AttrPrintLoc::Left;
>   else if (canPrintOnLeftSide(A)) {
> // For functions with body defined we print the attributes on the left
> // side so that GCC accept our dumps as well.
> if (const FunctionDecl *FD = dyn_cast(D);
> FD && FD->isThisDeclarationADefinition())
>   // In case Decl is a function with a body, then attrs should be 
> print
>   // on the left side.
>   AttrLoc = AttrPrintLoc::Left;
> 
>   // In case it is a variable declaration with a ctor, then allow
>   // printing on the left side for readbility.
> else if (const VarDecl *VD = dyn_cast(D);
>VD && VD->getInit() &&
>VD->getInitStyle() == VarDecl::CallInit)
>   AttrLoc = AttrPrintLoc::Left;
>   }
>   // Only print the side matches the user requested.
>   if ((Loc & AttrLoc) != AttrPrintLoc::None)
> A->printPretty(Out, Policy);
> ```
> 
> As you can see, both `if (const FunctionDecl *FD` and `else if (const VarDecl 
> *VD` are inside the  `if (canPrintOnLeftSide())`, and the `AttrLoc` variable 
> is only set to `AttrPrintLoc::Left` when this function returns `true`. Hence 
> unless there is a bug in `canPrintOnLeftSide` or `mustPrintOnLeftSide` (which 
> I don't see any), I can't see a case where PrintOnLeft is false and it prints 
> on the left side.
> 
> Now, I could be wrong on this, but IIRC the brackets on this is optional once 
> the `else if` would match the preceding else-less if, which in this case 
> would be `if (const FunctionDecl *FD = dyn_cast(D);`, which is 
> what the indentation meant.
yep, you're right, it WAS already behaving how it was supposed to.  Committed.  
Please keep an eye out for failed buildbots/etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG46f3ade5083b: Fix ast print of variables with attributes 
(authored by giulianobelinassi, committed by erichkeane).

Changed prior to commit:
  https://reviews.llvm.org/D141714?vs=556197=556201#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/CMakeLists.txt
  clang/lib/AST/DeclPrinter.cpp
  clang/test/AST/ast-print-attr-knr.c
  clang/test/AST/ast-print-attr.c
  clang/test/AST/ast-print-method-decl.cpp
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/Analysis/blocks.mm
  clang/test/OpenMP/assumes_codegen.cpp
  clang/test/OpenMP/assumes_print.cpp
  clang/test/OpenMP/assumes_template_print.cpp
  clang/test/OpenMP/declare_simd_ast_print.cpp
  clang/test/Sema/attr-print.c
  clang/test/SemaCXX/attr-print.cpp
  clang/test/SemaCXX/cxx11-attr-print.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp
  clang/utils/TableGen/TableGen.cpp
  clang/utils/TableGen/TableGenBackends.h

Index: clang/utils/TableGen/TableGenBackends.h
===
--- clang/utils/TableGen/TableGenBackends.h
+++ clang/utils/TableGen/TableGenBackends.h
@@ -39,6 +39,8 @@
 void EmitClangAttrClass(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrImpl(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrList(llvm::RecordKeeper , llvm::raw_ostream );
+void EmitClangAttrPrintList(const std::string ,
+llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrSubjectMatchRuleList(llvm::RecordKeeper ,
llvm::raw_ostream );
 void EmitClangAttrPCHRead(llvm::RecordKeeper , llvm::raw_ostream );
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -31,6 +31,8 @@
   GenClangAttrSubjectMatchRulesParserStringSwitches,
   GenClangAttrImpl,
   GenClangAttrList,
+  GenClangAttrCanPrintLeftList,
+  GenClangAttrMustPrintLeftList,
   GenClangAttrDocTable,
   GenClangAttrSubjectMatchRuleList,
   GenClangAttrPCHRead,
@@ -127,6 +129,14 @@
"Generate clang attribute implementations"),
 clEnumValN(GenClangAttrList, "gen-clang-attr-list",
"Generate a clang attribute list"),
+clEnumValN(GenClangAttrCanPrintLeftList,
+   "gen-clang-attr-can-print-left-list",
+   "Generate list of attributes that can be printed on left "
+   "side of a decl"),
+clEnumValN(GenClangAttrMustPrintLeftList,
+   "gen-clang-attr-must-print-left-list",
+   "Generate list of attributes that must be printed on left "
+   "side of a decl"),
 clEnumValN(GenClangAttrDocTable, "gen-clang-attr-doc-table",
"Generate a table of attribute documentation"),
 clEnumValN(GenClangAttrSubjectMatchRuleList,
@@ -315,6 +325,12 @@
   case GenClangAttrList:
 EmitClangAttrList(Records, OS);
 break;
+  case GenClangAttrCanPrintLeftList:
+EmitClangAttrPrintList("CanPrintOnLeft", Records, OS);
+break;
+  case GenClangAttrMustPrintLeftList:
+EmitClangAttrPrintList("PrintOnLeft", Records, OS);
+break;
   case GenClangAttrDocTable:
 EmitClangAttrDocTable(Records, OS);
 break;
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3207,6 +3207,27 @@
   OS << "#undef PRAGMA_SPELLING_ATTR\n";
 }
 
+// Emits the enumeration list for attributes.
+void EmitClangAttrPrintList(const std::string , RecordKeeper ,
+raw_ostream ) {
+  emitSourceFileHeader(
+  "List of attributes that can be print on the left side of a decl", OS);
+
+  AttrClassHierarchy Hierarchy(Records);
+
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
+  std::vector PragmaAttrs;
+  for (auto *Attr : Attrs) {
+if (!Attr->getValueAsBit("ASTNode"))
+  continue;
+
+if (!Attr->getValueAsBit(FieldName))
+  continue;
+
+OS << "case attr::" << Attr->getName() << ":\n";
+  }
+}
+
 // Emits the enumeration list for attributes.
 void EmitClangAttrSubjectMatchRuleList(RecordKeeper , raw_ostream ) {
   emitSourceFileHeader(
Index: clang/test/SemaCXX/cxx11-attr-print.cpp
===
--- clang/test/SemaCXX/cxx11-attr-print.cpp
+++ clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid 

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/DeclPrinter.cpp:312
+  // printing on the left side for readbility.
+else if (const VarDecl *VD = dyn_cast(D);
+   VD && VD->getInit() &&

While compiling this, I discovered that the lack of curley braces on the `else 
if (canPrintOnLeftSide(A))` means that this `else-if` is actually an else to 
THAT instead, despite its indention (which implies it should be associated with 
the `if (const FunctionDecl...`).

Which is it?  I presume the indenting is what you mean, but it hasn't been 
tested in a way that matters.  Can you add a test that validates the var-decl 
printing on the LHS ONLY happens when it 'can' print on the LHS?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D141714#4640825 , 
@giulianobelinassi wrote:

> In D141714#4638225 , @erichkeane 
> wrote:
>
>> Looks fine to me, please don't commit for a day or two to give 
>> @aaron.ballman a chance to make a final comment.
>
> I am sorry, but how am I supposed to commit those changes to main if I do not 
> have write permission?

I didn't realize you didn't have commit permissions!  If you send me (either 
here, or privately) your name and email in the form of: "Full Name 
"  I can do it for you


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D159126#4640484 , @cor3ntin wrote:

> Fix libc++ compile
>
> @erichkeane I struggle to reduce further, do you think it's worth adding this 
> inscrutable 
> test case somewhere?

There's definitely value to putting it in somewhere, I'd suggest giving 
meaningful names to the identifiers however.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159126/new/

https://reviews.llvm.org/D159126

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaLambda.cpp:2263
+LocalInstantiationScope )
+: FunctionScope(SemasRef) {
+  if (!isLambdaCallOperator(FD)) {

cor3ntin wrote:
> erichkeane wrote:
> > cor3ntin wrote:
> > > erichkeane wrote:
> > > > Wonder if `LambdaScopeForCallOperatorInstantiationRAII` should just 
> > > > inherit from `FunctionScopeRAII? Am I missing why not?
> > > I suppose it could yes. I think it would have to be protected so that 
> > > `disable` is not misused 
> > Ah, this isn't the object I thought it was (the actual scope), just the 
> > RAII, so I think this is all right/not much benefit to it.
> > 
> > I was afraid it was the actual 'scope' object, so introspection into the 
> > list of active scopes would pull us into some private variable/init 
> > somewhere rather than the ctor of the containing object.  No reason to 
> > change this now.
> Oups, I missed your reply and changed it. Hopefully you are still happy with 
> it!
Yep, no problem with it now, a bit of a brainfart when I suggested it in the 
first place.  Still LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159126/new/

https://reviews.llvm.org/D159126

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Sema/SemaLambda.cpp:2263
+LocalInstantiationScope )
+: FunctionScope(SemasRef) {
+  if (!isLambdaCallOperator(FD)) {

cor3ntin wrote:
> erichkeane wrote:
> > Wonder if `LambdaScopeForCallOperatorInstantiationRAII` should just inherit 
> > from `FunctionScopeRAII? Am I missing why not?
> I suppose it could yes. I think it would have to be protected so that 
> `disable` is not misused 
Ah, this isn't the object I thought it was (the actual scope), just the RAII, 
so I think this is all right/not much benefit to it.

I was afraid it was the actual 'scope' object, so introspection into the list 
of active scopes would pull us into some private variable/init somewhere rather 
than the ctor of the containing object.  No reason to change this now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159126/new/

https://reviews.llvm.org/D159126

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

1 suggestion, else LGTM.




Comment at: clang/lib/Sema/SemaLambda.cpp:2263
+LocalInstantiationScope )
+: FunctionScope(SemasRef) {
+  if (!isLambdaCallOperator(FD)) {

Wonder if `LambdaScopeForCallOperatorInstantiationRAII` should just inherit 
from `FunctionScopeRAII? Am I missing why not?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159126/new/

https://reviews.llvm.org/D159126

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Looks fine to me, please don't commit for a day or two to give @aaron.ballman a 
chance to make a final comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159358: [Clang] Realize generic lambda call operators are dependent sooner

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

This is great! Probably worth the cherry pick to Clang17 as well!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159358/new/

https://reviews.llvm.org/D159358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158061: [clang] Construct ExprRequirement with SubstitutionDiagnostic on SubstFailure

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Clean up the test layout a little as a part of the commit, otherwise, LGTM!




Comment at: clang/test/SemaCXX/concept-crash-on-diagnostic.cpp:28
+
+// expected-error@#1 {{no member named 'value' in 'is_convertible'}}
+

So I typically don't do the 'error' with a bookmark, just put the 'error' next 
to the line that causes the error, and bookmark the note.  It makes it easier 
to read.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158061/new/

https://reviews.llvm.org/D158061

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141714: Fix ast print of variables with attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think we can do better naming the tablegen'ed parts, else a bunch of smaller 
changes.  Approach seems good enough to me, though Aaron should scroll 
through/make a determination after you've fixed my concerns.




Comment at: clang/include/clang/Basic/Attr.td:565
 class Attr {
+  // On a declaration, can this attribute be print on the left side?
+  bit CanPrintOnLeftSide = 1;





Comment at: clang/include/clang/Basic/Attr.td:566
+  // On a declaration, can this attribute be print on the left side?
+  bit CanPrintOnLeftSide = 1;
+  // On a declaration, must this attribute be print on the left side?





Comment at: clang/include/clang/Basic/Attr.td:567
+  bit CanPrintOnLeftSide = 1;
+  // On a declaration, must this attribute be print on the left side?
+  bit MustPrintOnLeftSide = 0;





Comment at: clang/include/clang/Basic/Attr.td:568
+  // On a declaration, must this attribute be print on the left side?
+  bit MustPrintOnLeftSide = 0;
   // The various ways in which an attribute can be spelled in source





Comment at: clang/lib/AST/DeclPrinter.cpp:263
+static bool canPrintOnLeftSide(const Attr *A) {
+  if (A->isStandardAttributeSyntax()) {
+return false;

Don't include curley brackets here.



Comment at: clang/lib/AST/DeclPrinter.cpp:280
+static bool mustPrintOnLeftSide(const Attr *A) {
+  if (A->isDeclspecAttribute()) {
+return true;

Same here, don't use curleys on single-liners.



Comment at: clang/lib/AST/DeclPrinter.cpp:298
+
+  AttrPrintLoc attrloc = AttrPrintLoc::Right;
+  if (mustPrintOnLeftSide(A)) {





Comment at: clang/lib/AST/DeclPrinter.cpp:306
+// side so that GCC accept our dumps as well.
+if (const FunctionDecl *FD = dyn_cast(D);
+FD && FD->isThisDeclarationADefinition()) {





Comment at: clang/lib/AST/DeclPrinter.cpp:314
+  // printing on the left side for readbility.
+} else if (const VarDecl *VD = dyn_cast(D);
+   VD && VD->getInit() &&





Comment at: clang/lib/AST/DeclPrinter.cpp:1003
 
-  printDeclType(T, (isa(D) && Policy.CleanUglifiedParameters &&
-D->getIdentifier())
-   ? D->getIdentifier()->deuglifiedName()
-   : D->getName());
+  std::string Name;
+

Instead of making this a std::string, make it a StringRef and just assign it on 
line 1005, it looks like both sides of that return a StringRef (deuglifiedName 
and getName), and your += is unnecessary (since there is nothing in Name here 
anyway).



Comment at: clang/utils/TableGen/TableGen.cpp:282
"Generate riscv_vector_builtin_sema.inc for clang"),
-clEnumValN(GenRISCVSiFiveVectorBuiltins, 
"gen-riscv-sifive-vector-builtins",
+clEnumValN(GenRISCVSiFiveVectorBuiltins,
+   "gen-riscv-sifive-vector-builtins",

Unrelated changes, please remove.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158963: [CodeGen] Function multi-versioning: don't set comdat for internal linkage resolvers

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Needs a release note.  I don't have a problem with the patch, but don't really 
understand how COMDAT works/linking works, but since you're basically the 
person I'd tell someone else to ask, I guess I'll trust you know what you're 
doing :)

Please add the release note while committing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158963/new/

https://reviews.llvm.org/D158963

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158061: [clang] Construct ExprRequirement with SubstitutionDiagnostic on SubstFailure

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Hi, sorry for the delay in reviewing, I'm just coming back from an extended 
vacation.  This looks alright, except for the test.




Comment at: clang/test/SemaCXX/concept-crash-on-diagnostic.cpp:26
+// Consume remaining notes/errors.
+// expected-note@* 0+{{}}
+// expected-error@* 0+{{}}

Please don't do this, actually write out the remaining diagnostics.  Same with 
the notes above.

Additionally, please use the 'bookmark' feature of verify-consumer to make sure 
the diagnostics/notes are in proper order (which makes it easier to figure out 
when reviewing).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158061/new/

https://reviews.llvm.org/D158061

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

Still LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158557/new/

https://reviews.llvm.org/D158557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:10112
+  Specialization->addAttr(PrevDecl->getAttr());
+  Consumer.AssignInheritanceModel(Specialization);
+}

tahonermann wrote:
> I am concerned that moving the call to `Consumer.AssignInheritanceModel()` to 
> immediately after the creation of the node, but before it is populated might 
> be problematic. Previously, this call was still made before the node was 
> completely constructed (e.g., before `setTemplateSpecializationKind()` is 
> called for it). It is difficult to tell if any consumers might be dependent 
> on more of the definition being present. 
SO I think we still need to do this off of the definition, right?  Else if 
`PrevDecl` ends up being a declaration (followed later by a definition that has 
the attribute), we'll end up missing it?  Typically attributes are 'added' by 
later decls, so only the 'latest one' (though attributes can't be added after 
definition IIRC) has 'them all'.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158869/new/

https://reviews.llvm.org/D158869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

I see aaron mentioned the release note and is ok with it happening on commit, 
so I'll undo my 'needs revision'.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158557/new/

https://reviews.llvm.org/D158557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Mostly just nits (plus my hatred for C style casts :) ), I didn't see any 
issues with the approach.




Comment at: clang/include/clang/AST/ASTLambda.h:45
+  return isLambdaCallOperator(DC) &&
+ !cast(DC)->getType().isNull() &&
+ !cast(DC)->isExplicitObjectMemberFunction();

Under what cases does this end up being null?  It seems like this condition 
shouldn't be necessary, and it doesn't seem like we should have a case where we 
create a method/function without a type set?



Comment at: clang/lib/AST/Decl.cpp:3624
+unsigned FunctionDecl::getNumNonObjectParams() const {
+  return getNumParams() - (unsigned)hasCXXExplicitFunctionObjectParameter();
+}





Comment at: clang/lib/AST/Decl.cpp:3629
+  return getMinRequiredArguments() -
+ (hasCXXExplicitFunctionObjectParameter() ? 1 : 0);
+}





Comment at: clang/lib/CodeGen/CGExprCXX.cpp:72
+  if (const auto *M = dyn_cast(Op->getCalleeDecl()))
+ArgsToSkip = (unsigned)!M->isExplicitObjectMemberFunction();
+}





Comment at: clang/lib/Sema/SemaDeclCXX.cpp:14942
+  ExprBuilder  =
+  ExplicitObject ? (ExprBuilder &)*ExplicitObject : (ExprBuilder &)*This;
 

These casts should likely not be C-style/clobber casts.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:14965
+CastBuilder To(
+ExplicitObject ? (ExprBuilder &)*ExplicitObject
+   : (ExprBuilder &)*DerefThis,

same here



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15050
   if (!Invalid) {
 // Add a "return *this;"
+Expr *ThisExpr = (ExplicitObject  ? (ExprBuilder &)*ExplicitObject

here and elsewhere.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.

Ah, wait, still no release note!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158557/new/

https://reviews.llvm.org/D158557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I suggest doing the -511LL value from the original bug report in the tests as 
well, else, fine to me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158557/new/

https://reviews.llvm.org/D158557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Still think a codegen test for this example is VERY valuable here.




Comment at: clang/test/SemaCXX/constexpr-string.cpp:681
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wconstant-conversion"
+namespace GH64876 {

rather than suppress these, it makes sense to me to just mark them 
expected-warning.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158557/new/

https://reviews.llvm.org/D158557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158733: [Clang] Fix a crash when an invalid immediate function call appears in a cast

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

1 nit that the build bots caught, else I'm happy.




Comment at: clang/lib/Sema/SemaExpr.cpp:18364
+
+assert(FD && FD->isImmediateFunction &&
+   "could not find an immediate function in this expression");




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158733/new/

https://reviews.llvm.org/D158733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aeubanks wrote:
> erichkeane wrote:
> > aeubanks wrote:
> > > aaron.ballman wrote:
> > > > aeubanks wrote:
> > > > > aaron.ballman wrote:
> > > > > > aeubanks wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > What happens for tentative definitions where the value isn't 
> > > > > > > > known? e.g.,
> > > > > > > > ```
> > > > > > > > [[clang::unnamed_addr]] int i1, i2;
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > What happens if the types are similar but not the same?
> > > > > > > > ```
> > > > > > > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > > > > > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > Should we diagnose taking the address of such an attributed 
> > > > > > > > variable so users have some hope of spotting the non-conforming 
> > > > > > > > situations?
> > > > > > > > 
> > > > > > > > Does this attribute have impacts across translation unit 
> > > > > > > > boundaries (perhaps only when doing LTO) or only within a 
> > > > > > > > single TU?
> > > > > > > > 
> > > > > > > > What does this attribute do in C++ in the presence of 
> > > > > > > > constructors and destructors? e.g.,
> > > > > > > > ```
> > > > > > > > struct S {
> > > > > > > >   S();
> > > > > > > >   ~S();
> > > > > > > > };
> > > > > > > > 
> > > > > > > > [[clang::unnamed_addr]] S s1, s2; // Are these merged and 
> > > > > > > > there's only one ctor/dtor call?
> > > > > > > > ```
> > > > > > > globals are only mergeable if they're known to be constant and 
> > > > > > > have the same value/size. this can be done at compile time only 
> > > > > > > if the optimizer can see the constant values, or at link time
> > > > > > > 
> > > > > > > so nothing would happen in any of the cases you've given.
> > > > > > > 
> > > > > > > but yeah that does imply that we should warn when the attribute 
> > > > > > > is used on non const, non-POD globals. I'll update this patch to 
> > > > > > > do that
> > > > > > > 
> > > > > > > as mentioned in the description, we actually do want to take the 
> > > > > > > address of these globals for table-driven parsing, but we don't 
> > > > > > > care about identity equality
> > > > > > > globals are only mergeable if they're known to be constant and 
> > > > > > > have the same value/size. this can be done at compile time only 
> > > > > > > if the optimizer can see the constant values, or at link time
> > > > > > >
> > > > > > > so nothing would happen in any of the cases you've given.
> > > > > > 
> > > > > > A that's good to know. So I assume we *will* merge these?
> > > > > > 
> > > > > > ```
> > > > > > struct S {
> > > > > >   int i, j;
> > > > > >   float f;
> > > > > > };
> > > > > > 
> > > > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > > > [[clang::unnamed_addr]] const S s3 = s2;
> > > > > > ```
> > > > > > 
> > > > > > > but yeah that does imply that we should warn when the attribute 
> > > > > > > is used on non const, non-POD globals. I'll update this patch to 
> > > > > > > do that
> > > > > > 
> > > > > > Thank you, I think that will be more user-friendly
> > > > > > 
> > > > > > > as mentioned in the description, we actually do want to take the 
> > > > > > > address of these globals for table-driven parsing, but we don't 
> > > > > > > care about identity equality
> > > > > > 
> > > > > > Yeah, I still wonder if we want to diagnose just the same -- if the 
> > > > > > address is never taken, there's not really a way to notice the 
> > > > > > optimization, but if the address is taken, you basically get UB 
> > > > > > (and I think we should explicitly document it as such). Given how 
> > > > > > easy it is to accidentally take the address of something (like via 
> > > > > > a reference in C++), I think we should warn by default, but still 
> > > > > > have a warning group for folks who want to live life dangerously.
> > > > > > > globals are only mergeable if they're known to be constant and 
> > > > > > > have the same value/size. this can be done at compile time only 
> > > > > > > if the optimizer can see the constant values, or at link time
> > > > > > >
> > > > > > > so nothing would happen in any of the cases you've given.
> > > > > > 
> > > > > > A that's good to know. So I assume we *will* merge these?
> > > > > > 
> > > > > > ```
> > > > > > struct S {
> > > > > >   int i, j;
> > > > > >   float f;
> > > > > > };
> > > > > > 
> > > > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > > > [[clang::unnamed_addr]] const S s3 = s2;

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aeubanks wrote:
> aaron.ballman wrote:
> > aeubanks wrote:
> > > aaron.ballman wrote:
> > > > aeubanks wrote:
> > > > > aaron.ballman wrote:
> > > > > > What happens for tentative definitions where the value isn't known? 
> > > > > > e.g.,
> > > > > > ```
> > > > > > [[clang::unnamed_addr]] int i1, i2;
> > > > > > ```
> > > > > > 
> > > > > > What happens if the types are similar but not the same?
> > > > > > ```
> > > > > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > > > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > > > > ```
> > > > > > 
> > > > > > Should we diagnose taking the address of such an attributed 
> > > > > > variable so users have some hope of spotting the non-conforming 
> > > > > > situations?
> > > > > > 
> > > > > > Does this attribute have impacts across translation unit boundaries 
> > > > > > (perhaps only when doing LTO) or only within a single TU?
> > > > > > 
> > > > > > What does this attribute do in C++ in the presence of constructors 
> > > > > > and destructors? e.g.,
> > > > > > ```
> > > > > > struct S {
> > > > > >   S();
> > > > > >   ~S();
> > > > > > };
> > > > > > 
> > > > > > [[clang::unnamed_addr]] S s1, s2; // Are these merged and there's 
> > > > > > only one ctor/dtor call?
> > > > > > ```
> > > > > globals are only mergeable if they're known to be constant and have 
> > > > > the same value/size. this can be done at compile time only if the 
> > > > > optimizer can see the constant values, or at link time
> > > > > 
> > > > > so nothing would happen in any of the cases you've given.
> > > > > 
> > > > > but yeah that does imply that we should warn when the attribute is 
> > > > > used on non const, non-POD globals. I'll update this patch to do that
> > > > > 
> > > > > as mentioned in the description, we actually do want to take the 
> > > > > address of these globals for table-driven parsing, but we don't care 
> > > > > about identity equality
> > > > > globals are only mergeable if they're known to be constant and have 
> > > > > the same value/size. this can be done at compile time only if the 
> > > > > optimizer can see the constant values, or at link time
> > > > >
> > > > > so nothing would happen in any of the cases you've given.
> > > > 
> > > > A that's good to know. So I assume we *will* merge these?
> > > > 
> > > > ```
> > > > struct S {
> > > >   int i, j;
> > > >   float f;
> > > > };
> > > > 
> > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > [[clang::unnamed_addr]] const S s3 = s2;
> > > > ```
> > > > 
> > > > > but yeah that does imply that we should warn when the attribute is 
> > > > > used on non const, non-POD globals. I'll update this patch to do that
> > > > 
> > > > Thank you, I think that will be more user-friendly
> > > > 
> > > > > as mentioned in the description, we actually do want to take the 
> > > > > address of these globals for table-driven parsing, but we don't care 
> > > > > about identity equality
> > > > 
> > > > Yeah, I still wonder if we want to diagnose just the same -- if the 
> > > > address is never taken, there's not really a way to notice the 
> > > > optimization, but if the address is taken, you basically get UB (and I 
> > > > think we should explicitly document it as such). Given how easy it is 
> > > > to accidentally take the address of something (like via a reference in 
> > > > C++), I think we should warn by default, but still have a warning group 
> > > > for folks who want to live life dangerously.
> > > > > globals are only mergeable if they're known to be constant and have 
> > > > > the same value/size. this can be done at compile time only if the 
> > > > > optimizer can see the constant values, or at link time
> > > > >
> > > > > so nothing would happen in any of the cases you've given.
> > > > 
> > > > A that's good to know. So I assume we *will* merge these?
> > > > 
> > > > ```
> > > > struct S {
> > > >   int i, j;
> > > >   float f;
> > > > };
> > > > 
> > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > [[clang::unnamed_addr]] const S s3 = s2;
> > > > ```
> > > yeah those are merged even just by clang
> > > 
> > > ```
> > > struct S {
> > >   int i, j;
> > >   float f;
> > > };
> > > 
> > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > [[clang::unnamed_addr]] const S s3 = s2;
> > > 
> > > const void * f1() {
> > >   return 
> > > }
> > > 
> > > const void * f2() {
> > >   return 
> > > }
> > > 
> > > const void * f3() {
> > >   return 
> > > }
> > 

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D158666#4611494 , @eandrews wrote:

> In D158666#4611481 , @erichkeane 
> wrote:
>
>> I think the .ifunc spelling was an oversight on my part when I implemented 
>> this, I didn't spend enough time investigating GCC's behavior when 
>> implementing this feature.  I think the alias is the right way about it, but 
>> I think the .ifunc should be the alias (at least as far as I can think it 
>> through right now). I think that works better because it supports a case 
>> where the 'definition' of the target-clones function is generated with GCC, 
>> but the 'caller' (also with target clones) comes from clang.  I THINK that 
>> makes more sense? But perhaps try to chart out the behavior of the GCC/Clang 
>> "Knows about TC"/"Doesn't know about TC" in each situation to see which are 
>> troublesome?
>>
>> Additionally, this needs a release note.
>
> Thanks for taking a look! Can you explain why we need an alias? As in, if we 
> just remove the .ifunc suffix in the 'ifunc' function here, it should work 
> without an alias I think. I have to re-check but IIRC this is what GCC does

At least short term, we have to have both .ifunc and  
exposed, else we end up breaking SOMEONE.  At the moment, we're breaking the 
example you have, however if we just switch the .ifunc to be , 
we end up breaking cases where 1 TU is built with Clang 18 and the other with 
Clang-trunk.  So we need to figure out what the 'compatibility story' is 
between Clang 18, Clang-Trunk, and GCC with each of them in the position of:

1- Generated the function TU
2- Consumed the function, not knowing about it being TC
3- Consumed the function, knowing it was TC.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158666/new/

https://reviews.llvm.org/D158666

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.

Accepted before I thought about the other order, so requesting changes so we 
can think this through... 1 thing we DO need to consider is how this works with 
OLDER versions of Clang/clang's behavior.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158666/new/

https://reviews.llvm.org/D158666

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

I think the .ifunc spelling was an oversight on my part when I implemented 
this, I didn't spend enough time investigating GCC's behavior when implementing 
this feature.  I think the alias is the right way about it, but I think the 
.ifunc should be the alias (at least as far as I can think it through right 
now). I think that works better because it supports a case where the 
'definition' of the target-clones function is generated with GCC, but the 
'caller' (also with target clones) comes from clang.  I THINK that makes more 
sense? But perhaps try to chart out the behavior of the GCC/Clang "Knows about 
TC"/"Doesn't know about TC" in each situation to see which are troublesome?

Additionally, this needs a release note.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158666/new/

https://reviews.llvm.org/D158666

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158433: [Clang] Do not change the type of captured vars when checking lambda constraints

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I agree with this approach, and think we should backport this if at all 
possible.  The original solution is a bit problematic, and I don't believe this 
solution is particularly risky.




Comment at: clang/lib/Sema/SemaConcept.cpp:727
+  } else
+FuncScope.disable();
+

curleys required here.



Comment at: clang/lib/Sema/SemaConcept.cpp:922
+  } else
 FuncScope.disable();
 

here too



Comment at: clang/lib/Sema/SemaExpr.cpp:19754
+
+// When evaluating some attributes (like enable_if) we might refer to a
+// function parameter appertaining to the same declaration as that

cor3ntin wrote:
> shafik wrote:
> > Why move this block of code for?
> `isVariableAlreadyCapturedInScopeInfo` is kinda badly name because it will 
> adjust `DeclRefType` by adding `const` as necessary.
> 
> if we capture a parameter, and then use it in a concept - which are not 
> checked from within the scope of the lambda, we need to add const to it, 
> which we can only do by reordering these paths. It's a bit subtle, and could 
> do with some improvement as I'm not sure parameters will always be const 
> correct in attributes currently. 
> But I tried to do a minimal fix
> 
> 
> 
> 
Yeah, this function is unfortunately doing a lot of work and is a bit of a mess 
unfortunately.  I think I see the logic of the reorder, but its a sensitive 
enough function that a minimal fix is prudent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158433/new/

https://reviews.llvm.org/D158433

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think there is value to ensuring we diagnose the negative-to-size_t 
conversion here, but I also think there is value to a CodeGen test to ensure we 
emit the correct value.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158557/new/

https://reviews.llvm.org/D158557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158616: To allow RAV to visit initializer when bitfield and initializer both given

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This needs a test and a release note, and it failed pre-commit CI.  I don't 
have a good idea how to test this however, there is perhaps one of the visitor 
unit tests that can show this off?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158616/new/

https://reviews.llvm.org/D158616

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Are there any Sema tests we can add to show that we warn/diagnose/SOMETHING on 
these?  If someone passes a negative size, we should probably at least do the 
warning that it was converted/truncated.




Comment at: clang/lib/AST/ExprConstant.cpp:9357
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))

Context missing throughout


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158557/new/

https://reviews.llvm.org/D158557

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155858: Add a concept AST node.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So I did a quick run through, I think this is a fine idea and an improvement.  
However, @sammccall has given some very good feedback that I'd like to have him 
do the final approval.




Comment at: clang/include/clang/AST/ASTConcept.h:213
   Expr *ImmediatelyDeclaredConstraint = nullptr;
+  ConceptReference *CR;
 

Preference to name this something like `ConceptRef` instead of `CR` here.



Comment at: clang/include/clang/AST/ExprConcepts.h:49
 protected:
+  ConceptReference *CR;
+

Same comment here.



Comment at: clang/include/clang/AST/ExprConcepts.h:149
 // there may not be a template argument list.
-return ArgsAsWritten->RAngleLoc.isValid() ? ArgsAsWritten->RAngleLoc
-  : ConceptName.getEndLoc();
+return CR->hasExplicitTemplateArgs() &&
+   CR->getTemplateArgsAsWritten()->getRAngleLoc().isValid()

What did you find that resulted in this change?  What does 
`hasExplicitTemplateArgs` 'mean' when we have empty arguments, so something 
like:

template C>
void foo(const C&){}

Basically, the end loc should ALWAYS be the right-angle if it exists, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155858/new/

https://reviews.llvm.org/D155858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155858: Add a concept AST node.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Hi!  I was away on vacation, so I didn't get a chance to take a look at this.  
I'm currently digging my way through my 'back to work' tasks, but will keep 
this on my list of things to review ASAP.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155858/new/

https://reviews.llvm.org/D155858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Cant help with the RT/LLVM parts, but the clang parts are pretty much right.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13278
+llvm::Value *
+CodeGenFunction::EmitX86CpuSupports(std::array FeatureMask) {
   Value *Result = Builder.getTrue();

Hmm... I guess size-wise this is on the edge of "const ref vs pass by value".  
I think its fine now, but 'next time' this grows we'll have to think about 
making this a const-ref.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13311
+  llvm::Constant *CpuFeatures2 =
+  CGM.CreateRuntimeVariable(ATy, "__cpu_features2");
+  cast(CpuFeatures2)->setDSOLocal(true);

This won't double-create this if used more than 1x, right?  There doesn't need 
to be something like GetOrCreate... here?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13272
+  uint64_t Mask = llvm::X86::getCpuSupportsMask(FeatureStrs);
+  uint32_t FeaturesMask[4] = {uint32_t(Mask), uint32_t(Mask >> 32), 0, 0};
+  return EmitX86CpuSupports(FeaturesMask);

MaskRay wrote:
> pengfei wrote:
> > Use `Lo_32(Mask)`, `Hi_32(Mask)`?
> Lo_32/Hi_32 are currently unused in clang/. Using this needs MathExtras.h. I 
> am unsure it's worthwhile..
I was previously using Lo_32/Hi_32 here (see the removed parts of 
EmitX86CpuSupports).  Were we using those transitively?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158329/new/

https://reviews.llvm.org/D158329

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13277
+llvm::Value *
+CodeGenFunction::EmitX86CpuSupports(const uint32_t FeaturesMask[4]) {
   Value *Result = Builder.getTrue();

Can this be a std::array instead?  The C array is pretty meaningless here.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13314
+  continue;
+if (!CpuFeatures2) {
+  CpuFeatures2 = CGM.CreateRuntimeVariable(ATy, "__cpu_features2");

Why is this in the loop?  Is there a good reason to not just always initialize 
it?  



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2688
+if (Arch.consume_front("x86-64")) {
+  if (Arch.empty()) // FEATURE_X86_64_BASELINE 95=2*32+31
+Mask[2] = 1u << 31;

Can you clarify the comments here on these?  I'm a little confused by their 
meaning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158329/new/

https://reviews.llvm.org/D158329

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157762: [WIP] Implement [[msvc::no_unique_address]]

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

still on vaca, but doing a drive-by.




Comment at: clang/include/clang/Basic/Attr.td:1798
+def NoUniqueAddressMSVC : InheritableAttr, 
TargetSpecificAttr {
+  let Spellings = [CXX11<"msvc", "no_unique_address", 201803>];
+  let Subjects = SubjectList<[NonBitField], ErrorDiag>;

aaron.ballman wrote:
> I was a bit shocked to learn that this really is the correct return value for 
> `__has_cpp_attribute`: https://godbolt.org/z/MW9q1hPEh
Thats... interestinghorrifying... else?



Comment at: clang/include/clang/Basic/Attr.td:1797
 
+def NoUniqueAddressMSVC : InheritableAttr, 
TargetSpecificAttr {
+  let Spellings = [CXX11<"msvc", "no_unique_address", 201803>];

I think I would combine this and the one above as a single attribute.  Then, 
only 'add' it in SemaDeclAttr.cpp when the spelling is the appropriate one for 
the current target.  IMO, msvc::no_unique_address should ONLY be valid in MSVC 
mode, else we encourage its propagation.

We don't need it to be a 'TargetSpecificAttr', just do the 'ignored' warning in 
the handler in SemaDeclAttr.td.

The accessor that we need is to just query which spelling.



Comment at: clang/lib/AST/Decl.cpp:4477
 
+bool FieldDecl::hasNoUniqueAddress() const {
+  return hasAttr() || hasAttr();

This ends up going away if we combine them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157762/new/

https://reviews.llvm.org/D157762

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Sorry for the delay in review, I'm still on vacation.  However, I don't like 
the idea of removing the satisfaction.  I have to think about it more, but 
optimally we'd just not add the detail until we knew what the RHS value was, 
though our design doesn't particularly support that currently.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157526/new/

https://reviews.llvm.org/D157526

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153892: [NFC] Initialize class member pointers to nullptr.

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Test failure is clang-format on ASTConsumers.cpp.  Fix that please (whatever it 
wants?) otherwise, LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153892/new/

https://reviews.llvm.org/D153892

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105759: Implement P2361 Unevaluated string literals

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Just 2 small nits, otherwise this all LGTM.




Comment at: clang/lib/Parse/ParseDecl.cpp:430
+}
+if (Expr.isInvalid()) {
+  SawError = true;

Please put a newline between unchained 'if' statements... it makes tehse really 
hard to read without it.

It happens a few times here.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:878
 
-  if (!isIntOrBool(AL.getArgAsExpr(0))) {
+  Expr *First = AL.getArgAsExpr(0);
+  if (!isIntOrBool(First)) {

Unrelated change here?  What is this for?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105759/new/

https://reviews.llvm.org/D105759

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Thanks for this!  I have no concerns from what I can see, but I see the others 
are doing a very thorough review, so I'll let them do the approvals.




Comment at: clang/include/clang/AST/DeclCXX.h:1911
   setRangeEnd(EndLocation);
-setIsCopyDeductionCandidate(false);
+setDeductionCandidateKind(DeductionCandidateKind::Normal);
   }

ychen wrote:
> cor3ntin wrote:
> > I'm wondering if the constructor should take a `DeductionCandidateKind` 
> > defaulted to normal here. All the places where it's set seem to be 
> > immediately after construction.
> That's true indeed. The awkward aspect is that the 
> `CXXDeductionGuideDecl::Create` call is far from `setDeductionCandidateKind`; 
> making `CXXDeductionGuideDecl::Create` takes a `DeductionCandidateKind` would 
> several other less related functions takes `DeductionCandidateKind` also.
I'm in favor of having this be a part of CXXDeductionGuideDecl::Create and the 
ctor as a parameter instead as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139837/new/

https://reviews.llvm.org/D139837

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D153296#4449089 , @yronglin wrote:

> Can we both check `SS->getCond()->containsErrors()` ? Maybe it can avoid 
> bitint's effect. WDYT? @erichkeane @shafik

I'm a TOUCH uncomfortable in that this only fixes the 'current' issue, and is a 
little fragile perhaps?  The problem we have is that the 'Value' is the 
incorrect size, which I presume we can get to a couple of ways?  I'd like to 
make sure we fix as many of those problems as possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D153296#4446016 , @shafik wrote:

> In D153296#612 , @erichkeane 
> wrote:
>
>> So I think I'm pretty confident that the only time we would call 
>> `EvaluateDependentExpr` is when we are in an error condition, so I'm 
>> convinced the fix 'as is' is incorrect.  The check for noteSideEffect 
>> records that we HAVE a side effect, then returns if we are OK ignoring them 
>> right now.
>>
>> So since we are in a state where ignoring this error-case is acceptable, I 
>> think returning early there is incorrect as well, at least from a 'code 
>> correctness' (even if there isn't a reproducer that would matter?).  I think 
>> we're in a case where we want to continue in order to ensure we go through 
>> the entire flow, so I THINK we should treat this as 'we have a value we 
>> don't know, so its just not found', and should fall into the check on 5019 
>> (unless of course, there is a 'default' option!).
>>
>> So I think that we should be checking if `Value` is valid right after the 
>> default check, which lets us fall into the 'default' branch and get 
>> additional diagnostics/continued evaluation.  WDYT @shafik / @yronglin ?
>
> I do see what you are saying but I think that we have similar cases where 
> without a value the next step is impossible to do for example in 
> `EvaluateStmt` the `case Stmt::ReturnStmtClass:` case:
>
>   case Stmt::ReturnStmtClass: {
>  const Expr *RetExpr = cast(S)->getRetValue();
>  FullExpressionRAII Scope(Info);
>  if (RetExpr && RetExpr->isValueDependent()) {
>EvaluateDependentExpr(RetExpr, Info);
>// We know we returned, but we don't know what the value is.
>return ESR_Failed;
>  }
>
> We can't return a value we can't calculate right? and here as well for the 
> `Stmt::DoStmtClass` case
>
>   if (DS->getCond()->isValueDependent()) {
>  EvaluateDependentExpr(DS->getCond(), Info);
>  // Bailout as we don't know whether to keep going or terminate the loop.
>  return ESR_Failed;
>}
>
> This case feels the same as the two above, we can't calculate the jump for 
> the switch if we can't calculate the value.
>
> CC @rsmith

In the return-case I think it makes sense to skip out right away, I'm less 
convinced in the 'doStmt' case.  Either way, I think we can get a somewhat 
'better' diagnostic by continuing here, which is my point.  We COULD start 
essentially compling with error-limit=1, but there is value to continuing when 
we can.  And in this case, I think it is logical to continue.

In D153296#4446662 , @yronglin wrote:

> In D153296#612 , @erichkeane 
> wrote:
>
>> So I think I'm pretty confident that the only time we would call 
>> `EvaluateDependentExpr` is when we are in an error condition, so I'm 
>> convinced the fix 'as is' is incorrect.  The check for noteSideEffect 
>> records that we HAVE a side effect, then returns if we are OK ignoring them 
>> right now.
>>
>> So since we are in a state where ignoring this error-case is acceptable, I 
>> think returning early there is incorrect as well, at least from a 'code 
>> correctness' (even if there isn't a reproducer that would matter?).  I think 
>> we're in a case where we want to continue in order to ensure we go through 
>> the entire flow, so I THINK we should treat this as 'we have a value we 
>> don't know, so its just not found', and should fall into the check on 5019 
>> (unless of course, there is a 'default' option!).
>>
>> So I think that we should be checking if `Value` is valid right after the 
>> default check, which lets us fall into the 'default' branch and get 
>> additional diagnostics/continued evaluation.  WDYT @shafik / @yronglin ?
>
> Erich, do you mean we do a modification like this？If I'm not misunderstand, I 
> think this looks good to me, we can get more diagnostics.
>
>   diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
>   index f1f89122d4cc..967695c61df5 100644
>   --- a/clang/lib/AST/ExprConstant.cpp
>   +++ b/clang/lib/AST/ExprConstant.cpp
>   @@ -4984,8 +4984,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult 
> , EvalInfo ,
>  return ESR_Failed;
>if (SS->getCond()->isValueDependent()) {
>  // Stop evaluate if condition expression contains errors.
>   -  if (SS->getCond()->containsErrors() ||
>   -  !EvaluateDependentExpr(SS->getCond(), Info))
>   +  if (!EvaluateDependentExpr(SS->getCond(), Info))
>return ESR_Failed;
>} else {
>  if (!EvaluateInteger(SS->getCond(), Value, Info))
>   @@ -4995,6 +4994,8 @@ static EvalStmtResult EvaluateSwitch(StmtResult 
> , EvalInfo ,
>  return ESR_Failed;
>  }
>
>   +  bool CondHasSideEffects = SS->getCond()->HasSideEffects(Info.getCtx());
>   +
>  // Find the switch case 

[PATCH] D153702: [Clang] Implement P2738R1 - constexpr cast from void*

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:8920
+  if (HasValidResult)
+CCEDiag(E, diag::note_constexpr_invalid_void_star_cast)
+<< SubExpr->getType() << Info.getLangOpts().CPlusPlus26

Does this diagnostic work?  In the case of the `!C++26`, the arity of the 
message is different, right?  Do you have a test for this in not c++26 mode 
that you can post? I Am not sure we do 'fill ins' for discarded 'select' 
branches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153702/new/

https://reviews.llvm.org/D153702

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151696: [x86] Remove CPU_SPECIFIC* MACROs and add getCPUDispatchMangling

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think this is OK, I have a slight fear we're losing a bit of the 'tune' 
functionality, but it is not impossible that we've never really cared about 
that.  One concern I have is that the list was used for the resolver function, 
but I don't see any test changes for that?  Are we properly filtering out the 
features list somehow?




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2272
   // favor this processor.
-  TuneCPU = getTarget().getCPUSpecificTuneName(
-  SD->getCPUName(GD.getMultiVersionIndex())->getName());
+  TuneCPU = SD->getCPUName(GD.getMultiVersionIndex())->getName();
 }

So my understanding here is that our intent was that the 'tune' cpu and the 
'selected' cpu were not necessarily the same (either not the same name, OR not 
the same CPU!), right?  Is that being lost here?  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151696/new/

https://reviews.llvm.org/D151696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151753: [Clang][Sema] Do not try to analyze dependent alignment during -Wcast-align

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Needs a release note, else LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151753/new/

https://reviews.llvm.org/D151753

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So I think I'm pretty confident that the only time we would call 
`EvaluateDependentExpr` is when we are in an error condition, so I'm convinced 
the fix 'as is' is incorrect.  The check for noteSideEffect records that we 
HAVE a side effect, then returns if we are OK ignoring them right now.

So since we are in a state where ignoring this error-case is acceptable, I 
think returning early there is incorrect as well, at least from a 'code 
correctness' (even if there isn't a reproducer that would matter?).  I think 
we're in a case where we want to continue in order to ensure we go through the 
entire flow, so I THINK we should treat this as 'we have a value we don't know, 
so its just not found', and should fall into the check on 5019 (unless of 
course, there is a 'default' option!).

So I think that we should be checking if `Value` is valid right after the 
default check, which lets us fall into the 'default' branch and get additional 
diagnostics/continued evaluation.  WDYT @shafik / @yronglin ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:2024
   if (const VarDecl *VD = dyn_cast(D)) {
+if (D->isPlaceholderVar())
+  return !VD->hasInit();

Why would we get here?  Doesn't line 1995 pick this up?  Or am I missing where 
D is modified?

ALSO, I'd suggest making this VD->isPlaceholderVar, as to not annoy the SA 
tools.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:725
 
+void Sema::NotePlaceholderVariableDefinition(SourceLocation Loc) {
+  Diag(Loc, getLangOpts().CPlusPlus26

Not a fan of this function name... Perhaps Diag instead of Note?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153536/new/

https://reviews.llvm.org/D153536

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4989
 if (SS->getCond()->isValueDependent()) {
   if (!EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

shafik wrote:
> As far as I can tell `Value` will still not be set if we don't return here 
> and we will still crash when we attempt to compare `Value` below:
> 
> ```
> LHS <= Value && Value <= RHS
> ```
I don't understand the comment?   We're returning 'failed', aren't we?  Except 
now `EvaluateDependentExpr`is where the failure is coming from?



Comment at: clang/lib/AST/ExprConstant.cpp:4893
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa(E))
+return false;

yronglin wrote:
> yronglin wrote:
> > erichkeane wrote:
> > > I'd probably suggest `E->containsErrors()` instead, to cover cases where 
> > > we're not the 'root' of a recovery expr?  So something like:
> > > 
> > > `switch(func_call(unknown_value))`
> > > 
> > > should create a dependent call expr, but would still contain errors.
> > Thanks! Use `E->containsErrors()` and added into release note.
> Seems check error inside `EvaluateDependentExpr` will missing diagnostic 
> messages.
> 
> This case was introduced in D84637
> ```
> constexpr int test5() { // expected-error {{constexpr function never produce}}
>   for (;; a++); // expected-error {{use of undeclared identifier}}  \
>expected-note {{constexpr evaluation hit maximum step 
> limit; possible infinite loop?}}
>   return 1;
> }
> ```
> ```
> ./main.cpp:2:11: error: use of undeclared identifier 'a'
> 2 |   for (;; a++); // expected-error {{use of undeclared identifier}}  \
>   |   ^
> 1 error generated.
> ```
> But I think the `infinite loop` diagnostic is unnecessary, should we update 
> the test case? WDYT?
Huh... thats interesting.  The 'Info.noteSideEffect' is sufficient to do that?  
Looking closer, I wonder if this is the wrong fix and his original idea was 
better.  It seems that this already has a 'containsError' check at the end, and 
should if it doesn't have side effects.  

I was hoping to have the problem generalized, but I think I was wrong, and we 
should go back to just fixing the 'switch' statements.



Comment at: clang/lib/AST/ExprConstant.cpp:4988
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

shafik wrote:
> yronglin wrote:
> > erichkeane wrote:
> > > It seems to me that the 'better' solution is to make 
> > > EvaluateDependentExpr (or one of its children) be RecoveryExpr aware, and 
> > > result in a failed value instead.  That way we get this 'fix' for more 
> > > than just switch statements.
> > Thanks for your review!
> Erich so there are places in `ExprConstant.cpp` where if we  
> `isValueDependent()` we bail out like in the `Stmt::ReturnStmtClass` case 
> inside `EvaluateStmt1()`. The gist I get from the comment there is 
> 
> ```cpp
> // We know we returned, but we don't know what the value is.
> ```
> 
> Is that not correct or does it depend on each specific case?
TBH, i don't have a good idea of that.  I spent a little time digging here, but 
the constant evaluator isn't my forte.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153589: [NFC] Initialize pointer fields and remove a needless null check.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:4156
   // CFG generation for unevaluated operands.
-  if (S && !S->isTypeDependent() && S->isPotentiallyEvaluated())
+  if (!S->isTypeDependent() && S->isPotentiallyEvaluated())
 return VisitChildren(S);

schittir wrote:
> erichkeane wrote:
> > schittir wrote:
> > > erichkeane wrote:
> > > > I get that we're counting on the dereference on 4145 to have made this 
> > > > check irrelevant, but are we sure that we KNOW that "S" is non-null 
> > > > here otherwise?  That is, is the bug actually 4145 doing 'alwaysAdd' 
> > > > without checking vs the unnecessary check here?
> > > VisitCXXTypeidExpr is used only in one place - here 
> > > https://github.com/llvm/llvm-project/blob/a89c9b35be08b665cc1a01d840bc20349ba1308f/clang/lib/Analysis/CFG.cpp#L2288
> > >  where S is not null. Null check for S already happens at the beginning 
> > > of the method where VisitCXXTypeidExpr is called. 
> > SG, an assert is perhaps not a bad idea, but LGTM.  I don't know very much 
> > about the CFG stuff, so aaron might wish to take a final look.
> What is SG? 
"Sounds Good"



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:58
   /// used as arguments will necessarily be available at construction time.
-  LazyRuntimeFunction()
-  : CGM(nullptr), FunctionName(nullptr), Function(nullptr) {}
+  LazyRuntimeFunction() {}
 

schittir wrote:
> erichkeane wrote:
> > 
> The comment above needs to be updated?
I don't think so?  It still leaves the class uninitialized.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153589/new/

https://reviews.llvm.org/D153589

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153589: [NFC] Initialize pointer fields and remove a needless null check.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:4156
   // CFG generation for unevaluated operands.
-  if (S && !S->isTypeDependent() && S->isPotentiallyEvaluated())
+  if (!S->isTypeDependent() && S->isPotentiallyEvaluated())
 return VisitChildren(S);

schittir wrote:
> erichkeane wrote:
> > I get that we're counting on the dereference on 4145 to have made this 
> > check irrelevant, but are we sure that we KNOW that "S" is non-null here 
> > otherwise?  That is, is the bug actually 4145 doing 'alwaysAdd' without 
> > checking vs the unnecessary check here?
> VisitCXXTypeidExpr is used only in one place - here 
> https://github.com/llvm/llvm-project/blob/a89c9b35be08b665cc1a01d840bc20349ba1308f/clang/lib/Analysis/CFG.cpp#L2288
>  where S is not null. Null check for S already happens at the beginning of 
> the method where VisitCXXTypeidExpr is called. 
SG, an assert is perhaps not a bad idea, but LGTM.  I don't know very much 
about the CFG stuff, so aaron might wish to take a final look.



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:58
   /// used as arguments will necessarily be available at construction time.
-  LazyRuntimeFunction()
-  : CGM(nullptr), FunctionName(nullptr), Function(nullptr) {}
+  LazyRuntimeFunction() {}
 




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153589/new/

https://reviews.llvm.org/D153589

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153589: [NFC] Initialize pointer fields and remove a needless null check.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:4156
   // CFG generation for unevaluated operands.
-  if (S && !S->isTypeDependent() && S->isPotentiallyEvaluated())
+  if (!S->isTypeDependent() && S->isPotentiallyEvaluated())
 return VisitChildren(S);

I get that we're counting on the dereference on 4145 to have made this check 
irrelevant, but are we sure that we KNOW that "S" is non-null here otherwise?  
That is, is the bug actually 4145 doing 'alwaysAdd' without checking vs the 
unnecessary check here?



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:59
   LazyRuntimeFunction()
-  : CGM(nullptr), FunctionName(nullptr), Function(nullptr) {}
+  : CGM(nullptr), FTy(nullptr), FunctionName(nullptr), Function(nullptr) {}
 

I would suggest instead making these inline initialization rather than here 
(that is, CGM = nullptr on line 49, etc).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153589/new/

https://reviews.llvm.org/D153589

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Just a rewording of the message, else LGTM.




Comment at: clang/docs/ReleaseNotes.rst:523
   (`#38717 _`).
+- Stop evaluate constant expression if the condition expression which in 
+  switch statement contains errors.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Also needs a release note.




Comment at: clang/lib/AST/ExprConstant.cpp:4893
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa(E))
+return false;

I'd probably suggest `E->containsErrors()` instead, to cover cases where we're 
not the 'root' of a recovery expr?  So something like:

`switch(func_call(unknown_value))`

should create a dependent call expr, but would still contain errors.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4988
+  if (SS->getCond()->containsErrors() ||
+  !EvaluateDependentExpr(SS->getCond(), Info))
 return ESR_Failed;

It seems to me that the 'better' solution is to make EvaluateDependentExpr (or 
one of its children) be RecoveryExpr aware, and result in a failed value 
instead.  That way we get this 'fix' for more than just switch statements.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153146: [CLANG] Fix potential integer overflow value in getRVVTypeSize()

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:9567
 
   unsigned EltSize = Context.getTypeSize(Info.ElementType);
   unsigned MinElts = Info.EC.getKnownMinValue();

I think  I'd suggest just changing the types of these two variables instead of 
the cast.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153146/new/

https://reviews.llvm.org/D153146

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152547: [clang][NFC] Drop alignment in builtin-nondeterministic-value test

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/test/CodeGen/builtins-nondeterministic-value.c:8
 // CHECK-LABEL: entry
-// CHECK: [[A:%.*]] = alloca i32, align 4
-// CHECK: store i32 [[X:%.*]], ptr [[A]], align 4
+// CHECK: [[A:%.*]] = alloca i32, align
+// CHECK: store i32 [[X:%.*]], ptr [[A]], align

this test doesn't seem to be testing for alignment at all, so just take out 
everything after the type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152547/new/

https://reviews.llvm.org/D152547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D150226#4408381 , @aaron.ballman 
wrote:

> In D150226#4404808 , @rupprecht 
> wrote:
>
>> I suppose including this warning in `ShowInSystemHeader` with your diff 
>> above would be a good first step anyway, right? If we're about to make it a 
>> hard error anyway, then making it a `ShowInSystemHeader` error first would 
>> ease that transition.
>
> Yes and no.
>
> If we're going to turn something into a hard error, letting folks 
> implementing system headers know about it is important, so from that 
> perspective, it makes sense to enable the diagnostic in system headers. 
> However, *users* have no choice in their system headers (oftentimes) which 
> makes the diagnostic unactionable for them as they're not going to (and 
> shouldn't have to) modify system headers, so from that perspective, enabling 
> the diagnostic in a system header introduces friction.
>
> If this diagnostic is showing up in system headers, I think we would likely 
> need to consider adding a compatibility hack to allow Clang to still consume 
> that system header while erroring when outside of that system header (we've 
> done this before to keep STL implementations working, for example). Between 
> this need and the friction it causes users to have an unactionable 
> diagnostic, I think we probably should not enable `ShowInSystemHeader`.

It seems to me that if our concern is breaking system headers, we need to do 
that with better testing.  Some sort of 'diagnostic group' for "This is going 
to become an error *SOON*" mixed with us/vendors running that on platforms we 
consider significant enough to not break.  But just diagnosing on arbitrary 
users with no choice on how to fix the headers doesn't seem appropriate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152335: [Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D152335#4404118 , @aaron.ballman 
wrote:

> LGTM with a release note.
>
> In D152335#4404114 , @erichkeane 
> wrote:
>
>> Needs a release note.  Also, the changing of order is POSSIBLY a change in 
>> behavior (in that we're now diagnosing 'out of range' before 'power of 2'), 
>> but I don't think we care.
>
> I don't consider that a behavioral change to be worried about. Telling the 
> user "this value is too big" before telling them "it's not a power of two" 
> seems equivalent or perhaps slightly better than "this is not a power of two" 
> and then "oh, and it's too big."

Well, its telling them "this value is too big" instead of "its not a power of 
two" rather than the other way around.  But yeah, i don't think I care?  I'm a 
little frightened there is no test that fails because of this, but not 
surprised.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152335/new/

https://reviews.llvm.org/D152335

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152335: [Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Needs a release note.  Also, the changing of order is POSSIBLY a change in 
behavior (in that we're now diagnosing 'out of range' before 'power of 2'), but 
I don't think we care.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152335/new/

https://reviews.llvm.org/D152335

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152197: Fix static analyzer bugs with null pointer dereferences in CheckSizelessVectorOperands()

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:11148
   if (LHSType->isVLSTBuiltinType() && RHSType->isVLSTBuiltinType() &&
+  LHSBuiltinTy && RHSBuiltinTy &&
   Context.getBuiltinVectorTypeInfo(LHSBuiltinTy).EC !=

I think this is unnecessary.  `isVLSTBuiltinType` only returns true if 
`LHSType` is a `BuiltinType` already (or at least, a subset-of). See : 
https://clang.llvm.org/doxygen/Type_8cpp_source.html#l02409




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152197/new/

https://reviews.llvm.org/D152197

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152335: [Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4484
+  if (!TooManyActiveBits) {
+AlignVal = Alignment.getZExtValue();
+// C++11 [dcl.align]p2:

So looking more closely, THIS is the problem right here.  I think we probably 
should just be storing `MaximumAlignment` and `AlignVal` as an llvm::APInt and 
just do the comparison on `APInt`, rather than try to be tricky with the bits 
here.

Basically, don't extract the 'Alignment' from the `APInt` until after the test 
for >.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152335/new/

https://reviews.llvm.org/D152335

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152335: [Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4480
 MaximumAlignment = std::min(MaximumAlignment, uint64_t(8192));
-  if (AlignVal > MaximumAlignment) {
+  bool TooManyActiveBits = Alignment.getActiveBits() > llvm::APInt(64, 
MaximumAlignment).getActiveBits();
+

What is the purpose of using 'ActiveBits' here?  Why is this not a sub-set of 
the operator `>` from below?  That is, 'ActiveBits' allows for 0xFF when the 
'MaxValue' is 0x80 (top bit set), but the operator > would have caught that 
anyway, right?  

Also, how does 'ActiveBits' work with negative numbers? 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152335/new/

https://reviews.llvm.org/D152335

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152083: [clang] Warning for uninitialized elements in fixed-size arrays

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Needs release notes and tests.  Patch needs full-context added.




Comment at: clang/include/clang/Basic/DiagnosticGroups.td:732
 def UninitializedConstReference : DiagGroup<"uninitialized-const-reference">;
+def UninitializedArrayElements : DiagGroup<"uninitialized-array-elements">;
 def Uninitialized  : DiagGroup<"uninitialized", [UninitializedSometimes,

philnik wrote:
> Why do you make this a new warning group instead of just adding it to 
> `-Wuninitialized`?
I think the new group makes sense, but it should be a part of Uninitialized as 
well (see 733).



Comment at: clang/lib/Sema/SemaInit.cpp:991
+Expr **inits = FullyStructuredList->getInits();
+for (unsigned i = 0, e = FullyStructuredList->getNumInits(); i != e;
+ ++i) {

should probably just do range-for here, use the .inits here instead.  Perhaps 
even better is a llvm::find of nullptr.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152083/new/

https://reviews.llvm.org/D152083

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152259: [Clang] Reject increment of bool value in unevaluated contexts after C++17

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D152259#4399781 , @yronglin wrote:

> Thanks for your review @erichkeane ! Seems the CI failure not caused by this 
> patch.

I agree, I don't think it is related.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152259/new/

https://reviews.llvm.org/D152259

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152195: [Clang] Fix access of friend function in local class

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D152195#4399445 , @eandrews wrote:

> In D152195#4399251 , @erichkeane 
> wrote:
>
>> I think the patch looks fine, but this needs a release note.
>
> Just so I know - Does every bug fix need a release note now?

Just about every commit we expect to have a release note unless it is fixing a 
regression introduced since the last release, or is NFC.  So yes, every bug fix 
should have one.  If possible, add a link to the github issue (if it doesn't 
exist, obviously you don't have to).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152195/new/

https://reviews.llvm.org/D152195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152069: [1/11][Clang][Type] Expand BuiltinTypeBits from 8 to 16 bits

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/AST/Type.h:1652
 /// The kind (BuiltinType::Kind) of builtin type this is.
-unsigned Kind : 8;
+unsigned Kind : 16;
   };

It looks like the largest of the Bitfields in the union uses 32 bits (an 
unsigned), so this doesn't grow anything as far as I can tell, right?

Based on your requirements, I'd rather we do 1 of 2 things;  

1- Make this 9 bits (or 10, or whatever is NECESSARY).
2- Just make this an `unsigned`, which doesn't grow the size, and put a comment 
on it that we can steal bits from it if needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152069/new/

https://reviews.llvm.org/D152069

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152194: [NFC][CLANG] Fix nullptr dereference issue found by static analyzer tool

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:756
 if (ReceiverType->isObjCIdType() || ReceiverType->isObjCClassType() ||
 ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType, TrackedType)) {
   const ObjCInterfaceDecl *InterfaceDecl = TrackedType->getInterfaceDecl();

erichkeane wrote:
> Manna wrote:
> > We are assigning: ReceiverObjectPtrType = nullptr return value from getAs.
> > ```
> > const auto *ReceiverObjectPtrType =
> >   ReceiverType->getAs();
> > ```
> > 
> > Then we are dereferencing  nullptr ReceiverObjectPtrType when calling 
> > canAssignObjCInterfaces()
> This isn't NFC, as `ReceiverObjectPtrType` is only used here.  If the 
> `MessageExpr` `ReceiverKind` is not `Instance` or `Class`, we never 
> dereference this.  So the declaration should be in this branch.
Oh, and ALSO, we don't dereference it if `ReceiverType` is `ObjCIdType` or 
`ObhjCClassType`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152194/new/

https://reviews.llvm.org/D152194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152194: [NFC][CLANG] Fix nullptr dereference issue found by static analyzer tool

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:756
 if (ReceiverType->isObjCIdType() || ReceiverType->isObjCClassType() ||
 ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType, TrackedType)) {
   const ObjCInterfaceDecl *InterfaceDecl = TrackedType->getInterfaceDecl();

Manna wrote:
> We are assigning: ReceiverObjectPtrType = nullptr return value from getAs.
> ```
> const auto *ReceiverObjectPtrType =
>   ReceiverType->getAs();
> ```
> 
> Then we are dereferencing  nullptr ReceiverObjectPtrType when calling 
> canAssignObjCInterfaces()
This isn't NFC, as `ReceiverObjectPtrType` is only used here.  If the 
`MessageExpr` `ReceiverKind` is not `Instance` or `Class`, we never dereference 
this.  So the declaration should be in this branch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152194/new/

https://reviews.llvm.org/D152194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152195: [Clang] Fix access of friend function in local class

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think the patch looks fine, but this needs a release note.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152195/new/

https://reviews.llvm.org/D152195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152197: [NFC][CLANG] Fix static analyzer bugs with null pointer dereferences in CheckSizelessVectorOperands()

[Erich Keane via Phabricator via cfe-commits]

erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.

The SA tool is incorrect here.




Comment at: clang/lib/Sema/SemaExpr.cpp:9
   if ((OperationKind == ACK_Arithmetic) &&
   ((LHSBuiltinTy && LHSBuiltinTy->isSVEBool()) ||
(RHSBuiltinTy && RHSBuiltinTy->isSVEBool( {

sdesmalen wrote:
> This doesn't seem like a non-functional change. Here it checks that 
> LHSBuiltinTy is not nullptr before using it, which suggests that it is valid 
> for LHSType/RHSType not to be a builtin type. There is also nothing else 
> guarding that the types are all VLSTBuiltinTypes. Do none of the tests fail?
In fact they DO!  Pre-commit CI found them.  



Comment at: clang/lib/Sema/SemaExpr.cpp:11148
   if (LHSType->isVLSTBuiltinType() && RHSType->isVLSTBuiltinType() &&
   Context.getBuiltinVectorTypeInfo(LHSBuiltinTy).EC !=
   Context.getBuiltinVectorTypeInfo(RHSBuiltinTy).EC) {

This is the only other use of htem, but the 1st part of the 'if' checks if 
these are builtin types.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152197/new/

https://reviews.llvm.org/D152197

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152259: [Clang] Make increment bool SFINAE-friendly

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This should have a release note.

I think the bug is asking to make this a SFINAE failure (it isn't clear what 
you mean by "SFINAE Friendly").  Our mechanism for that is to mark the 
diagnostic SFINAEError.




Comment at: clang/lib/Sema/SemaExpr.cpp:14603
+  // Ensure increment bool SFINAE-friendly.
+  if (S.isUnevaluatedContext() || S.isSFINAEContext())
+return QualType();

I think what you really need to do is just set `ext_increment_bool` to be 
`SFINAEError` in `DiagnosticSemaKinds.td`, right?  Not all of this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152259/new/

https://reviews.llvm.org/D152259

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152186: [Clang] Add release note for 877210faa447

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

2 nits, otherwise LGTM!  Thanks!




Comment at: clang/docs/ReleaseNotes.rst:333
   can be controlled using ``-fcaret-diagnostics-max-lines=``.
+- Clang no longer emits ``-Wunused-variable`` for variables declared with
+  ``__attribute__((cleanup(...)))`` to match GCC.





Comment at: clang/docs/ReleaseNotes.rst:334
+- Clang no longer emits ``-Wunused-variable`` for variables declared with
+  ``__attribute__((cleanup(...)))`` to match GCC.
 




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152186/new/

https://reviews.llvm.org/D152186

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152180: [Sema] Do not emit -Wunused-variable for variables declared with cleanup attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I note I forgot to require a release note, so please add one of those (and make 
sure it gets incorporated in any cherry-pick that happens).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152180/new/

https://reviews.llvm.org/D152180

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151575: [clang][diagnostics] Always show include stacks on errors

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I don't have much of an opinion on the commit itself.  It seems that 
suppressing that include stack does at least SOME work to make our error-novels 
less 'War and Peace', but I don't really get the bug report well enough to know 
whether we should be doing this.




Comment at: clang/lib/Frontend/DiagnosticRenderer.cpp:175
   // Skip redundant include stacks altogether.
   if (LastIncludeLoc == IncludeLoc)
 return;

I'd probably just implement this as 

`if (Level != DiagnosticsEngine::Error && LastIncludeLoc == IncludeLoc)`, 
rather than the 2 + an assignment.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151575/new/

https://reviews.llvm.org/D151575

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151121: [Clang][UBSan] Fix the crash caused by __builtin_assume_aligned with -no-opaque-pointers enabled

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I don't know much about this section, but I DO note that the pre-commit CI 
failures all seem related here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151121/new/

https://reviews.llvm.org/D151121

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This all LGTM, but I want @tahonermann to do a pass as well, he knows more 
about FP than I do, so I don't want to accept without his run-through.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149573/new/

https://reviews.llvm.org/D149573

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127762: [Clang][AArch64] Add/implement ACLE keywords for SME.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think we need a few tests to ensure that these are properly propagated on 
function templates (both for Sema and codegen).  Also, a handful of nits (See 
my list).




Comment at: clang/include/clang/AST/Type.h:3958
+SME_PStateZAPreservedMask = 1 << 3,
+SME_AttributeMask = 63 // We only support maximum 6 bits because of the
+   // bitmask in FunctionTypeExtraBitfields.





Comment at: clang/include/clang/Basic/Attr.td:2459
+def ArmPreservesZA : TypeAttr, TargetSpecificAttr {
+  let Spellings = [RegularKeyword<"__arm_preserves_za">];
+  let Subjects = SubjectList<[HasFunctionProto], ErrorDiag>;

So, why are `__arm_shared_za` and `__arm_preserves_za` different tenses, 
despite being very similar sounding?  Should it be `__arm_shares_za` (or 
alternatively, `__arm_preserved_za`)?



Comment at: clang/include/clang/Basic/AttrDocs.td:6595
+
+* the function requires the Scalable Matrix Extension (SME)
+

I'm missing a touch of context that an additional word or two might help.  is 
this 'the function requires the 'target' processor has SME?  or the 'runtime' 
processor?

Also, the rest of the list ends in a '.', but this one doesnt.



Comment at: clang/include/clang/Basic/AttrDocs.td:6633
+It applies to function types and specifies that the function shares
+ZA state with its caller.  This means that:
+

"ZA" should be defined 1st time it is used in this doc, and the ones below.



Comment at: clang/include/clang/Basic/AttrDocs.td:6635
+
+* the function requires the Scalable Matrix Extension (SME)
+

Same comments as above.



Comment at: clang/include/clang/Basic/AttrDocs.td:6679
+
+* the function requires the Scalable Matrix Extension (SME)
+

Same here.



Comment at: clang/include/clang/Basic/AttrDocs.td:6707
+
+* the function requires the Scalable Matrix Extension (SME)
+

again.



Comment at: clang/lib/Sema/SemaDecl.cpp:3758
 
+  // It is not allowed to redeclare an SME function with different SME
+  // attributes.





Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9444
+  case ParsedAttr::AT_ArmNewZA:
+if (auto *FPT = dyn_cast(D->getFunctionType())) {
+  if (FPT->getAArch64SMEAttributes() &

This switch should ONLY be 'handle' function calls.  Please break this off into 
its own function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127762/new/

https://reviews.llvm.org/D127762

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152140: [Clang] Limit FunctionTypeExtraBitfields::NumExceptionType to 16 bits.

[Erich Keane via Phabricator via cfe-commits]

erichkeane requested changes to this revision.
erichkeane added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/AST/Type.cpp:3374
 auto  = *getTrailingObjects();
-ExtraBits.NumExceptionType = epi.ExceptionSpec.Exceptions.size();
+auto NumExceptions = epi.ExceptionSpec.Exceptions.size();
+assert(NumExceptions <= UINT16_MAX &&

So this isn't a place where we're allowed to use `auto`.  We can only use it if 
the type is listed on the RHS (like in the case of casts/etc).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152140/new/

https://reviews.llvm.org/D152140

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits