[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

Note that the Clang 16 branch is coming up approximately on January 24, and 
this needs to be reverted or have the issue reported by @steven_wu fixed by 
then.  @mizvekov : If you or someone else don't have a solution/revert to this 
by January 13th, @aaron.ballman and I will begin the process to revert this 
ourselves.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D136565: [clang] Instantiate alias templates with sugar

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

Note that the Clang 16 branch is coming up approximately on January 24, and 
this needs to be reverted or perf-regression fixed by then.  @mizvekov : If you 
or someone else don't have a solution/revert to this by January 13th, 
@aaron.ballman and I will begin the process to revert this ourselves.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136565

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

[Lldb-commits] [PATCH] D136565: [clang] Instantiate alias templates with sugar

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D136565#4032150 , @erichkeane 
wrote:

> Note that the Clang 16 branch is coming up approximately on January 24, and 
> this needs to be reverted or perf-regression fixed by then.  @mizvekov : If 
> you or someone else don't have a solution/revert to this by January 13th, 
> @aaron.ballman and I will begin the process to revert this ourselves.

I see that this was already reverted in 
a5c18fcf6e7ffeea72aaf079477caf2ac1d641bc 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136565

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

@mizvekov : I'm looking at the revert of this since no work seems to have been 
done to fix @steven_wu 's problem.  Unfortunately, it seems that reverting this 
requires reverting https://reviews.llvm.org/D134604 plus all of the 
"Instantiate" patches, which undoes basically all of your work.

I'm starting it now, but its unfortunate that we are going to lose this much of 
your work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D131858#4047799 , @v.g.vassilev 
wrote:

> In D131858#4047785 , @erichkeane 
> wrote:
>
>> @mizvekov : I'm looking at the revert of this since no work seems to have 
>> been done to fix @steven_wu 's problem.  Unfortunately, it seems that 
>> reverting this requires reverting https://reviews.llvm.org/D134604 plus all 
>> of the "Instantiate" patches, which undoes basically all of your work.
>>
>> I'm starting it now, but its unfortunate that we are going to lose this much 
>> of your work.
>
> That is *really* unfortunate. @mizvekov (@erichkeane) is there any other way 
> out here?

@mizvekov said he was working on fixing the issue, and we gave him a deadline 
of Tomorrow for it.  If he (or someone else) manages to fix the problem by 
Tuesday early AM PT, I won't have to do that.  BUT we cannot release with this 
regression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D131858#4051336 , @aaron.ballman 
wrote:

> In D131858#4050112 , @rsmith wrote:
>
>> In D131858#3957630 , @arphaman 
>> wrote:
>>
>>> This change has caused a failure in Clang's stage 2 CI on the green dragon 
>>> Darwin CI: 
>>> https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6390/console.
>>>
>>>   Assertion failed: (lvaluePath->getType() == elemTy && "Unexpected type 
>>> reference!"), function readAPValue, file 
>>> /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/include/clang/AST/AbstractBasicReader.inc,
>>>  line 736.
>>
>> This assert is simply wrong, and I've removed it in 
>> rG2009f2450532450a99c1a03d5e2c30f478121839 
>>  -- 
>> that change should be safe to cherry-pick into the release branch. It's 
>> possible for the recomputation of the type after deserialization to result 
>> in a different type than what we saw when serializing, because 
>> redeclarations of the same entity can use the same type with different sugar 
>> -- or even slightly different types in some cases, such as when an array 
>> bound is added in a redeclaration. The dumps of the types provided by 
>> @steven_wu confirms that we were just seeing a difference in type sugar in 
>> this case.
>>
>>>   Assertion failed: (BlockScope.empty() && CurAbbrevs.empty() && "Block 
>>> imbalance"), function ~BitstreamWriter, file 
>>> /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/llvm/include/llvm/Bitstream/BitstreamWriter.h,
>>>  line 119.
>>
>> Is this still happening? If so, this looks more serious, and will need 
>> further investigation.
>>
>> Can we undo the workaround in https://reviews.llvm.org/D139956 and see if 
>> the bot is now happy? Or can someone who was seeing problems before 
>> (@steven_wu?) run a test?
>
> Thank you for poking at this Richard! However, I think we still need to 
> revert the functionality in this area unless we're able to make headway on 
> https://github.com/llvm/llvm-project/issues/59271 and quickly. FWIW, I ran 
> into this exact problem yesterday on my dev machine, so the overhead is still 
> a present concern. If that's something you plan to work on, then I think it'd 
> make sense for Erich to hold off on starting the revert work to give you a 
> chance to improve this. But if nobody is actively working on it, we need to 
> start pulling this back because the branch date is a bit over a week away 
> (Jan 24).

My understanding is that the submitter of that bug did sufficient analysis to 
determine that https://reviews.llvm.org/D136566 is the cause of his perf 
regression, having done an analysis the patch before and after.  The only 
reason to revert THIS patch (and the follow-ups, since this is a 'base patch' 
to the rest) is the report by @steven_wu .

SO, @steven_wu: Can ypu please, ASAP, try to reproduce your issue as Richard 
asked above? IF so, we only have to revert D136566 
, which should fix our performance issue. 
(that is, revert the workaround you submitted in 
https://reviews.llvm.org/D139956, then see if it works?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

@arphaman I see you're part of the original report too, if you could try to 
revert the workaround and test it, it would be nice too!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

Note, I just did the revert of the workaround myself here: 
498bf3424d011235d420e02e80e135fae646d537 


I won't see the failure on 
https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/ until Tuesday, but 
if it DOES fail like reported above, I'll continue on the effort to revert this 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D131858#4052026 , @v.g.vassilev 
wrote:

> Thanks a lot @rsmith for providing a fix and thanks a lot @aaron.ballman and 
> @erichkeane for the efforts saving @mizvekov work over the summer. I believe 
> he has sporadic access to internet and soon he will be back to normal. Great 
> example of team work here!!

Note we don't yet have the evidence that this is savable yet, we should know in 
the next day or so.  At minimum, at least 1 of his patches needs to be reverted 
due to the memory regression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D131858#4054348 , @v.g.vassilev 
wrote:

> In D131858#4052031 , @erichkeane 
> wrote:
>
>> In D131858#4052026 , @v.g.vassilev 
>> wrote:
>>
>>> Thanks a lot @rsmith for providing a fix and thanks a lot @aaron.ballman 
>>> and @erichkeane for the efforts saving @mizvekov work over the summer. I 
>>> believe he has sporadic access to internet and soon he will be back to 
>>> normal. Great example of team work here!!
>>
>> Note we don't yet have the evidence that this is savable yet, we should know 
>> in the next day or so.  At minimum, at least 1 of his patches needs to be 
>> reverted due to the memory regression.
>
> I see that https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6584/ 
> picked up your revert and seems successful. Then the next build is green as 
> well and then starts failing for a reason that’s unrelated to this patch.

Yep!  So only 1 of the patches (D136566  
needs reverting/further attention).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:3036
 if (!std::is_trivially_destructible::value) {
-  auto DestroyPtr = [](void *V) { static_cast(V)->~T(); };
-  AddDeallocation(DestroyPtr, Ptr);
+  auto DestroyPtr = [](void *V) { ((T *)V)->~T(); };
+  AddDeallocation(DestroyPtr, (void *)Ptr);

This change is weird... what is going on here?



Comment at: clang/include/clang/AST/TemplateBase.h:88
+/// so cannot be dependent.
+UncommonValue,
+

I definitely hate the name here... Just `Value` makes a bit more sense, but 
isn't perfectly accurate.  Perhaps `NonTypeValue`?  But that is a little 
redundant.  `Uncommon` here is just strange and not particularly descriptive. 



Comment at: clang/lib/AST/MicrosoftMangle.cpp:1670
+  case TemplateArgument::UncommonValue:
+if (ValueDecl *D = getAsArrayToPointerDecayedDecl(
+TA.getUncommonValueType(), TA.getAsUncommonValue())) {

Has microsoft implemented this yet?  Can we see what they chose to do and make 
sure we match them? 



Comment at: clang/lib/AST/ODRHash.cpp:177
+case TemplateArgument::UncommonValue:
+  // FIXME: Include a representation of these arguments.
   break;

I feel like this DEFINITELY needs to happen.  Nullptr/integral is less 
important, but now that we have an actual value here, I think it needs to be 
part of the hash.



Comment at: clang/lib/AST/TemplateBase.cpp:204-211
+  if (Type->isIntegralOrEnumerationType() && V.isInt())
+*this = TemplateArgument(Ctx, V.getInt(), Type);
+  else if ((V.isLValue() && V.isNullPointer()) ||
+   (V.isMemberPointer() && !V.getMemberPointerDecl()))
+*this = TemplateArgument(Type, /*isNullPtr=*/true);
+  else if (const ValueDecl *VD = getAsSimpleValueDeclRef(Ctx, Type, V))
+// FIXME: The Declaration form should expose a const ValueDecl*.

aaron.ballman wrote:
> Well this is certainly a unique approach...  Personally, I think it'd be nice 
> to not assign to `this` within a constructor by calling other constructor; 
> that falls squarely into "wait, what, can you even DO that?" kind of 
> questions for me.
I agree, this function needs to be refactored to call some sort of 'init' 
function or something, even if we have to refactor the other constructors.  
This assignment to `*this` is just too strange to let stay.



Comment at: clang/lib/AST/TemplateBase.cpp:619
+  case TemplateArgument::UncommonValue: {
+// FIXME: We're guessing at LangOptions!
+SmallString<32> Str;

aaron.ballman wrote:
> It's probably okay enough, but this is now the third instance of adding the 
> same bug to this helper method -- maybe we should fix that instead?
Agreed, seems to me we should do the work NOW to just wire in the lang-opts to 
this function.



Comment at: clang/lib/Sema/SemaTemplate.cpp:7882
+Sema &S, QualType T, const APValue &Val, SourceLocation Loc) {
+  auto MakeInitList = [&](ArrayRef Elts) -> Expr * {
+auto *ILE = new (S.Context) InitListExpr(S.Context, Loc, Elts, Loc);

I don't have a good idea of what is happening in this function here, it isn't 
really clear... before committing, someone needs to do a deep dive on this 
function for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:3036
 if (!std::is_trivially_destructible::value) {
-  auto DestroyPtr = [](void *V) { static_cast(V)->~T(); };
-  AddDeallocation(DestroyPtr, Ptr);
+  auto DestroyPtr = [](void *V) { ((T *)V)->~T(); };
+  AddDeallocation(DestroyPtr, (void *)Ptr);

bolshakov-a wrote:
> erichkeane wrote:
> > This change is weird... what is going on here?
> Here is not very beautiful attempt to workaround const-ness of 
> `TemplateArgument::V::Value` pointer passed here from the added 
> `TemplateArgument` constructor. The change in this line isn't acually needed 
> and made only for consistence with the next line, I think. Alternatively, I 
> can
> 1) refactor `addDestruction` and `AddDeallocation` to work with pointers to 
> constants, or
> 2) add `const_cast` to `AddDeallocation` call in the next line, or
> 3) make `TemplateArgument::V::Value` pointer non-const.
> 
> I'm biased to the first variant.
I'd lean towards #3, it ends up being consistent with the rest of the things 
here.  #1 is interesting, but that results in these functions violating 
const-correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

Patch seems to be missing all the context.


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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D140996#4189452 , @bolshakov-a 
wrote:

> Sorry! It's my first time using Phabricator. Maybe, the problem occurs 
> because I've solved the issue with Arcanist just by means of copy-pasting 
> patches into "Update Diff" Web GUI form. Maybe, I should reopen the PR?

When you generate your patch, you need to use -U99 to make sure you get 
full context.  I typically pipe it to a file (the git diff /git show HEAD), 
then use the upload file version.


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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added subscribers: eli.friedman, gribozavr2, akyrtzi, gribozavr.
erichkeane added inline comments.



Comment at: clang/include/clang/AST/TemplateBase.h:88
+/// so cannot be dependent.
+UncommonValue,
+

bolshakov-a wrote:
> shafik wrote:
> > erichkeane wrote:
> > > I definitely hate the name here... Just `Value` makes a bit more sense, 
> > > but isn't perfectly accurate.  Perhaps `NonTypeValue`?  But that is a 
> > > little redundant.  `Uncommon` here is just strange and not particularly 
> > > descriptive. 
> > This catch all `UncommonValue` feels artificial. Maybe we need a separate 
> > out the cases into more granular cases like `Float` etc
> @erichkeane, it looks strange, I agree. Even just `CommonValue` sounds better 
> for me (but my English is far from fluent). Maybe, `ArbitraryValue`?
> 
> @shafik, your suggestion would move this PR far enough from the original 
> Richard's commit. And I'd prefer to merge `Declaration`, `Integral`, and 
> `NullPtr` kinds into that is currently called `UncommonValue` rather than to 
> repeat here various `APValue` kinds.
I don't think splitting out the individual cases has all that much value, at 
least until we NEED it.  

As far as a name, what about `StructuralValue`?  P1907 calls the 'type' of that 
A 'structural type'? It isn't perfect, but it at least seems to be somewhat 
defensible with standards language.



Comment at: clang/lib/AST/ItaniumMangle.cpp:4397
+// argument.
+// As proposed in https://github.com/itanium-cxx-abi/cxx-abi/issues/111.
+auto *SNTTPE = cast(E);

aaron.ballman wrote:
> We should get this nailed down. It was proposed in Nov 2020 and the issue is 
> still open. CC @rjmccall 
This definitely needs to happen.  @rjmccall or @eli.friedman ^^ Any idea what 
the actual mangling should be?



Comment at: clang/lib/AST/MicrosoftMangle.cpp:1670
+  case TemplateArgument::UncommonValue:
+if (ValueDecl *D = getAsArrayToPointerDecayedDecl(
+TA.getUncommonValueType(), TA.getAsUncommonValue())) {

bolshakov-a wrote:
> erichkeane wrote:
> > Has microsoft implemented this yet?  Can we see what they chose to do and 
> > make sure we match them? 
> Experimentally, I've made me sure that MSVC produces the same mangled names 
> for the cases provided in the `mangle-ms-templates` test as in the test. But 
> there are problems with references to subobjects: some cases are explicitly 
> unsupported in `mangleTemplateArgValue`, and some cases produce a magled name 
> different from what MSVC does. Should it be fixed in this PR, or may be 
> delayed?
We need to end up doing our best to match the microsoft mangling if at all 
possible, since they own the ABI.  I DEFINITELY would want any followup patch 
to be promised for Clang17 (that is, we don't release Clang17 with this patch 
and NOT that patch), so I'd expect said patch to be available for review before 
this gets committed.

As far as whether it needs to happen in THIS patch, we can perhaps decide based 
on the severity of the break, if you can provide examples (or, if it is split 
into a separate patch, we can use the tests there).



Comment at: clang/lib/AST/TemplateBase.cpp:244
+  else if (const ValueDecl *VD = getAsSimpleValueDeclRef(Ctx, Type, V))
+// FIXME: The Declaration form should expose a const ValueDecl*.
+initFromDeclaration(const_cast(VD), Type, IsDefaulted);

Why can this not happen now?



Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+// FIXME: Visit value.
+break;

bolshakov-a wrote:
> aaron.ballman wrote:
> > Any particular reason this isn't being handled now?
> I need some guidance here. Which characters are allowed in the USR? Could the 
> mangling algorithm from `CXXNameMangler::mangleValueInTemplateArg` be moved 
> into some common place and reused here?
I have no idea what is valid here.  BUT @akyrtzi and @gribozavr (or @gribozavr2 
?) seem to be the ones that touch these files the most?



Comment at: clang/lib/Sema/SemaTemplate.cpp:7944
+  case APValue::Indeterminate:
+// FIXME: Are these values possible?
+  case APValue::LValue:

Rather than this, I'd prefer these all down to the llvm_unreachable below.  If 
we find they are reachable, then we'd be expected to implement them at that 
point.


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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added inline comments.



Comment at: clang/www/cxx_status.html:1067
+  Reference type template arguments referring to 
instantiation-dependent objects and subobjects
+  (i.e. delcared inside a template but neither type- nor 
value-dependent) aren't fully supported.
+




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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a subscriber: asl.
erichkeane added a comment.

I've got the 1 concern with the mangling that I REALLY want one of our codegen 
owners to chime in on, otherwise this LGTM.




Comment at: clang/lib/AST/ItaniumMangle.cpp:4397
+// argument.
+// As proposed in https://github.com/itanium-cxx-abi/cxx-abi/issues/111.
+auto *SNTTPE = cast(E);

erichkeane wrote:
> aaron.ballman wrote:
> > We should get this nailed down. It was proposed in Nov 2020 and the issue 
> > is still open. CC @rjmccall 
> This definitely needs to happen.  @rjmccall or @eli.friedman ^^ Any idea what 
> the actual mangling should be?
This is still an open, and we need @rjmccall @eli.friedman or @asl to help out 
here.


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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

What is the justification for this?  Do other compilers do this?  Was there an 
RFC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151683

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

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D151683#4382408 , @philnik wrote:

> In D151683#4380877 , @erichkeane 
> wrote:
>
>> What is the justification for this?
>
> What exactly are you asking for? Why I'd like to back port it? This would 
> make quite a bit of code in libc++ simpler and avoids pit-falls where an 
> attribute works in some place in some version but not in another.
>
>> Do other compilers do this?
>
> ICC and NVC++ support this: https://godbolt.org/z/TeMnGdGsY
>
>> Was there an RFC?
>
> No. I guess, since you are asking I should write one for this? Only for the 
> removal of `-fdouble-square-bracket-attributes`, or also for back porting 
> this?

The RFC I would expect for "allow C/C++ style attributes as an extension in 
previous modes".  This is a pretty significant feature to allow as an 
extension, particularly since our two main alternative compilers (G++/MSVC) 
don't have this as an extension. I'd be curious how the other C compilers do 
this as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151683

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via lldb-commits]

erichkeane accepted this revision.
erichkeane added a comment.

This LGTM, but please give @davrec time (a few days?) to do another review 
before merging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D134604: [clang] Implement sugared substitution changes to infrastructure

[Erich Keane via Phabricator via lldb-commits]

erichkeane accepted this revision.
erichkeane added a comment.

A couple of nits, otherwise I don't have to see this again.




Comment at: clang/include/clang/AST/TemplateName.h:158
+  // When true the substitution will be finalized (subst node won't be placed).
+  bool getFinal() const;
+

Based on this comment, should this be 'Finalize' instead of 'Final'?  Or 
something like "ShouldFinalize"?



Comment at: clang/include/clang/Sema/Template.h:104
 /// Construct a single-level template argument list.
-MultiLevelTemplateArgumentList(Decl *D, ArgList Args) {
-  addOuterTemplateArguments(D, Args);
+MultiLevelTemplateArgumentList(Decl *D, ArgList Args, bool Final) {
+  addOuterTemplateArguments(D, Args, Final);

Hrmph, this additional flag likely requires rebasing/rebuilding on a patch I 
just committed, which uses this MLTAL again.  Hopefully not too bad to 
propagate.



Comment at: clang/lib/AST/ASTContext.cpp:3045
+static bool
+getCanonicalTemplateArguments(const ASTContext &C,
+  ArrayRef OrigArgs,

typically we name these sorts of things based on their return type, so this 
would be more `hasCanonicalTemplateArguments`.  BUT the modifying nature of it 
makes this an awkward function all the same.

I think I'd suggest putting both of the 'output's here on the same side of the 
function, either both a return, or both an out-param, so the name isn't 
ambiguous/confusing like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134604

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.
Herald added subscribers: Michael137, JDevlieghere.

A few comments.  I don't mind the approach, though I find myself wondering if 
the "FormatDiagnostic" call should stay the same, and choose the legacy-reason 
only when a sarif reason doesn't exist?  Or for some level of command line 
control?

The clang-side interface to this seems a touch clunky, and I fear it'll make 
continuing its use/generalizing its use less likely.




Comment at: clang/include/clang/Basic/DiagnosticIDs.h:165
+struct DiagnosticReason {
+  std::string Legacy;
+  std::string SARIF;

"Reason" is a part of the diagnostic itself, pre-rendered, right? Since these 
strings are literals, can this be an llvm::StringLiteral instead? (the 
constexpr-stringref type)?

Else, generating these is going to cost us at startup time.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4520
+  DiagReason<
+/*Legacy:*/"invalid explicitly-specified argument for template parameter 
%0",
+/*SARIF:*/"we passed a %select{type|value|class template}1 as our 
%ordinal2 "

Already kidna hate this format here.  Is there any way we could make this be 
something more like:

`DiagReason, SARIF<"whatever">>` ?



Comment at: clang/lib/Format/TokenAnalyzer.cpp:47
   llvm::SmallVector Message;
-  Info.FormatDiagnostic(Message);
+  Info.FormatSummary(Message);
+  Info.FormatLegacyReason(Message);

This pair of calls has happened ~20 times by the time I've scrolled here.  Can 
we do a single call here of some sort instead?



Comment at: clang/test/Frontend/sarif-reason.cpp:15
+void g() {
+  f1<0>(); // expected-error{{no matching function for call to 'f1'}}
+  f1(); // expected-error{{no matching function for call to 'f1'}}

This is definitely a case where I'd love the diagnostics formatted/arranged 
differently here.  If you can use the #BOOKMARK style to make sure errors and 
notes are together, it would better illustrate what you're trying to do here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D138939#3958185 , @cjdb wrote:

>> The clang-side interface to this seems a touch clunky, and I fear it'll make 
>> continuing its use/generalizing its use less likely.
>
> Is this w.r.t. the `FormatDiagnostic` being split up, or is it something 
> else? If it's the former: I'll be changing that to `FormatLegacyDiagnostic`, 
> which //almost// gets us back to where we started.

Urgh, I was a bit afraid you'd ask that :D  It is more of a feeling I guess, 
which is perhaps biased by this patch being particularly in the 
diagnostics-handling code.  However, I suspect that over time, you're going to 
want to start switching all these uses of `FormatLegacyReason` over to 
`FormatSarifReason`, and I would want the 'easy way' to be the 'right' way in 
either case.  Having it be a 2-liner, or over-specifying what is otherwise the 
'default' behavior is a bit disconcerting.

In D138939#3958231 , @cjdb wrote:

>> though I find myself wondering if the "FormatDiagnostic" call should stay 
>> the same, and choose the legacy-reason only when a sarif reason doesn't 
>> exist? Or for some level of command line control?
>
> Hmm... isn't this the point of the diagnostic consumers?

I don't have a great feeling of that?  I haven't done much thinking into the 
diagnostics architecture over the years.  That said, the use of when we'd 
choose one vs the other isn't completely clear to me yet.




Comment at: clang/lib/Basic/Diagnostic.cpp:791
 
 /// FormatDiagnostic - Format this diagnostic into a string, substituting the
 /// formal arguments into the %0 slots.  The result is appended onto the Str

Comment no longer matches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D138939#3963473 , @tschuett wrote:

> Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, DiagnosticMode 
> mode)`instead of `void FormatDiagnostic(SmallVectorImpl &OutStr)`?
> To make the transition easer and future proof.

I like this idea.  Perhaps with DiagnosticMode being a 3-way enum:

  enum struct DiagnosticMode {
Legacy,
Sarif,  
Default = Legacy
  }

I like the idea in particular, since it makes a command line flag to modify 
"Default" to be whichever the user prefers pretty trivial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D138939#3964404 , @cjdb wrote:

> In D138939#3963496 , @erichkeane 
> wrote:
>
>> In D138939#3963473 , @tschuett 
>> wrote:
>>
>>> Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, DiagnosticMode 
>>> mode)`instead of `void FormatDiagnostic(SmallVectorImpl &OutStr)`?
>>> To make the transition easer and future proof.
>>
>> I like this idea.  Perhaps with DiagnosticMode being a 3-way enum:
>>
>>   enum struct DiagnosticMode {
>> Legacy,
>> Sarif,  
>> Default = Legacy
>>   }
>>
>> I like the idea in particular, since it makes a command line flag to modify 
>> "Default" to be whichever the user prefers pretty trivial.
>
> There's already a flag for this: `-fdiagnostics-format=sarif`. Why do we need 
> a second diagnostic mode flag?

Ah, oh... is the Sarif formatting being done with a new formatter?  That seems 
unfortunate, since folks using the other formatters won't be able to use the 
user friendly formats.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D138939#3964439 , @erichkeane 
wrote:

> In D138939#3964404 , @cjdb wrote:
>
>> In D138939#3963496 , @erichkeane 
>> wrote:
>>
>>> In D138939#3963473 , @tschuett 
>>> wrote:
>>>
 Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, DiagnosticMode 
 mode)`instead of `void FormatDiagnostic(SmallVectorImpl &OutStr)`?
 To make the transition easer and future proof.
>>>
>>> I like this idea.  Perhaps with DiagnosticMode being a 3-way enum:
>>>
>>>   enum struct DiagnosticMode {
>>> Legacy,
>>> Sarif,  
>>> Default = Legacy
>>>   }
>>>
>>> I like the idea in particular, since it makes a command line flag to modify 
>>> "Default" to be whichever the user prefers pretty trivial.
>>
>> There's already a flag for this: `-fdiagnostics-format=sarif`. Why do we 
>> need a second diagnostic mode flag?
>
> Ah, oh... is the Sarif formatting being done with a new formatter?  That 
> seems unfortunate, since folks using the other formatters won't be able to 
> use the user friendly formats.

I've been alerted offline that I am misunderstanding the Sarif proposal, and 
where this is going.  I'll note that I wasn't present/invited at the calls 
where all of this was discussed, so I am admittedly not completely up to date.  
The above concern shouldn't stop others from reviewing this, particularly if 
you better understand the intent here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added inline comments.



Comment at: clang/test/Frontend/sarif-reason.cpp:15
+void g() {
+  f1<0>(); // expected-error{{no matching function for call to 'f1'}}
+  f1(); // expected-error{{no matching function for call to 'f1'}}

cjdb wrote:
> erichkeane wrote:
> > This is definitely a case where I'd love the diagnostics formatted/arranged 
> > differently here.  If you can use the #BOOKMARK style to make sure errors 
> > and notes are together, it would better illustrate what you're trying to do 
> > here.
> This is maybe done? I'm not sure if this is the #BOOKMARK style you're 
> referring to, but it should capture the same intent. Lemme know if you had 
> something else in mind and I'll happily change it 🙂 
It isn't exactly (in that it is using line-numbers instead of bookmarks), but 
the ordering is fine for me.

The bookmarking is something like:

```
LineThatHasNote; // #NoteLine
...
LineThatCausesError;
// expected-error@-1 {{Some Error}}
// expected-note@#NoteLine {{The Note}}
```

However, what I REALLY care about is that the notes and errors are 'next' to 
eachother, since they are easier to read that way




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D84126: Enable -Wsuggest-override in the LLVM build

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

Ive got a bit a of a problem here: This flag (suggest-override) on GCC 8.3 (and 
others) warns on missing-override EVEN WHEN the function is marked 'final'.  
This was not fixed until GCC 9.2: https://godbolt.org/z/55KeM3

The result is my build gave a few thousand diagnostics for this.  I'd suggest 
disabling this unless the GCC version is >9.1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84126



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