[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[Xiang Zhang via Phabricator via cfe-commits]

xiangzhangllvm added inline comments.



Comment at: llvm/lib/Analysis/ConstantFolding.cpp:101
 /// Constant fold bitcast, symbolically evaluating it with DataLayout.
 /// This always returns a non-null constant, but it may be a
 /// ConstantExpr if unfoldable.

clin1 wrote:
> xiangzhangllvm wrote:
> > clin1 wrote:
> > > API for this function always returns non-null: can we return 
> > > ConstantExpr::getBitCast(C,DestTy) instead? Then the change in SCCP is 
> > > not needed either.
> > I tried, the SCCP will also fold the bitcast into the following instruction.
> I see, that makes sense. But are we sure that all callers of FoldBitCast are 
> doing a null check: for example, FoldReinterpretLoadFromConstPtr calls 
> FoldBitCast several times, and null is not checked before dereference. Maybe 
> the AMX type cannot happen in this case?
> Alternative: can AMX be checked in SCCP?
Right! let me check the callers of FoldBitCast,
Luckly, there is only several callers of FoldBitCast, and almost all in this 
file ConstantFolding.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98757

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D98638#2630864 , @lxfind wrote:

> That's a fair point. I agree that we have no guarantee these are the only two 
> cases.
> It does seem to me that coroutine implementation somewhat relies on proper 
> lifetime markers so that data are being put correctly, which may be the 
> fundamental problem we are trying to solve.

It is hard to prove it. This topic need more discuss and more folks get 
involved. But it is really valuable. I can't remember how many patches we had 
to judge whether values should be put on the coroutine frame. I am OK to emit 
lifetime markers even at O0. But I think you need to ask for other's opinion.

In D98638#2630864 , @lxfind wrote:

> We probably could, but it would be very very tedious. 
> During CodeGen, we only have the AST that's calling __builtin_coro_resume, 
> which we will call Emit as a whole.
> So we need to manually match the AST 2 levels down to find the await_suspend 
> call, get its name, and then walk through the emitted IR to find a call with 
> the same name, and then find the tmp that's used to store the return value of 
> the call, and then emit llvm.coro.forcestack.

Can't we did as inline comments?




Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221
 CGF.EmitBlock(RealSuspendBlock);
+  } else if (ForcestackStart) {
+Builder.CreateCall(

can we rewrite it into:
```
else if (SuspendRet != nullptr && SuspendRet->getType()->isClassType()) {
 // generate:
 // llvm.coro.forcestack(SuspendRet)
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[Chang Lin via Phabricator via cfe-commits]

clin1 added inline comments.



Comment at: llvm/lib/Analysis/ConstantFolding.cpp:101
 /// Constant fold bitcast, symbolically evaluating it with DataLayout.
 /// This always returns a non-null constant, but it may be a
 /// ConstantExpr if unfoldable.

xiangzhangllvm wrote:
> clin1 wrote:
> > API for this function always returns non-null: can we return 
> > ConstantExpr::getBitCast(C,DestTy) instead? Then the change in SCCP is not 
> > needed either.
> I tried, the SCCP will also fold the bitcast into the following instruction.
I see, that makes sense. But are we sure that all callers of FoldBitCast are 
doing a null check: for example, FoldReinterpretLoadFromConstPtr calls 
FoldBitCast several times, and null is not checked before dereference. Maybe 
the AMX type cannot happen in this case?
Alternative: can AMX be checked in SCCP?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98757

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

[PATCH] D98726: [analyzer] Remove unnecessary TODO

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 331164.
RedDocMD added a comment.

Removed unnecessary include


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98726

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp


Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  
-analyzer-checker=core,cplusplus.Move,cplusplus.NewDelete,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
@@ -43,6 +43,14 @@
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
+void derefOfReleasedPtr(A *oldptr) {
+  std::unique_ptr P(oldptr);
+  A* aptr = P.release();
+  delete aptr; // expected-note {{Memory is released}}
+  aptr->foo(); // expected-warning {{Use of memory after it is freed 
[cplusplus.NewDelete]}}
+  // expected-note@-1{{Use of memory after it is freed}}
+}
+
 void derefAfterReset() {
   std::unique_ptr P(new A()); // expected-note {{Smart pointer 'P' is 
constructed}}
   P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -391,8 +391,6 @@
 checkAndPrettyPrintRegion(OS, ThisRegion);
 OS << " is released and set to null";
   }));
-  // TODO: Add support to enable MallocChecker to start tracking the raw
-  // pointer.
 }
 
 void SmartPtrModeling::handleSwap(const CallEvent ,


Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,cplusplus.NewDelete,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
@@ -43,6 +43,14 @@
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
+void derefOfReleasedPtr(A *oldptr) {
+  std::unique_ptr P(oldptr);
+  A* aptr = P.release();
+  delete aptr; // expected-note {{Memory is released}}
+  aptr->foo(); // expected-warning {{Use of memory after it is freed [cplusplus.NewDelete]}}
+  // expected-note@-1{{Use of memory after it is freed}}
+}
+
 void derefAfterReset() {
   std::unique_ptr P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
   P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -391,8 +391,6 @@
 checkAndPrettyPrintRegion(OS, ThisRegion);
 OS << " is released and set to null";
   }));
-  // TODO: Add support to enable MallocChecker to start tracking the raw
-  // pointer.
 }
 
 void SmartPtrModeling::handleSwap(const CallEvent ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

> Then if we want to put the result of the await_suspend in the stack, I think 
> we can do it under CodeGen part only. It should be easy to judge the return 
> type of await_suspend and create a call to llvm.coro.forcestack to the return 
> value of await_suspend.

We probably could, but it would be very very tedious. 
During CodeGen, we only have the AST that's calling __builtin_coro_resume, 
which we will call Emit as a whole.
So we need to manually match the AST 2 levels down to find the await_suspend 
call, get its name, and then walk through the emitted IR to find a call with 
the same name, and then find the tmp that's used to store the return value of 
the call, and then emit llvm.coro.forcestack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

> Then we need to answer the question: how can we **prove** that the result of 
> symmetric transfer and %gro are the **only** exceptions from the above rules. 
> Or how can we know the list of exceptions wouldn't get longer and longer in 
> the future?
>
> Then go back to the example in the summary. From my point of view, the key 
> problem is that our escape analysis isn't powerful enough. I don't ask us to 
> do excellent escape analysis. It may beyond our abilities. I just want to say 
> how can we know the result of symmetric transfer and %gro are the only 
> exceptions.

That's a fair point. I agree that we have no guarantee these are the only two 
cases.
It does seem to me that coroutine implementation somewhat relies on proper 
lifetime markers so that data are being put correctly, which may be the 
fundamental problem we are trying to solve.

> In D98638#2630778 , @lxfind wrote:
>
>> Whether or not the current coroutine frame would be destroyed completely 
>> depend on the implementation of await_suspend. So we cannot predict or know 
>> in advance. Therefore, the temporary handle returned by await_suspend must 
>> be put on the stack. I don't really see any other solutions other than this.
>
> OK. Although the main stream implementation of await_suspend only destroy the 
> coroutine handle in the final awaiter, the compiler can't assume the normal 
> await_suspend won't destroy it. So I agree to guard the result of the 
> await_suspend to make it put on the stack. At least, it would reduce the size 
> of the coroutine frame.
>
> Then if we want to put the result of the await_suspend in the stack, I think 
> we can do it under CodeGen part only. It should be easy to judge the return 
> type of await_suspend and create a call to llvm.coro.forcestack to the return 
> value of await_suspend.
>
> In D98638#2630778 , @lxfind wrote:
>
>> Well, I guess another potential solution is to force emitting lifetime 
>> intrinsics for this part of coroutine in the front-end.
>
> I am not sure if this is a good idea. May it break the guide principle in 
> LLVM? This need to be reviewed by others.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D93594: [X86] Pass to transform amx intrinsics to scalar operation.

[LuoYuanke via Phabricator via cfe-commits]

LuoYuanke added a comment.

I can reproduce the regression. I'll help to fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93594

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D98638#2630778 , @lxfind wrote:

> What do you think is the fundamental problem, though?

It is hard to give a formal description for the problem. Let me try to explain 
it.
What I want to say here is about rules that decide whether a value should be 
put on the coroutine frame.
Initially, we put values on the frame for whose uses are crossing suspend 
points with their definition.
Then, we put values on the frame for whose uses are crossing suspend points 
with their definition and uses are not escaped.
In this patch, we want to put values on the frame for whose uses are crossing 
suspend points with their definition and uses are not escaped but except the 
result of symmetric transfer and %gro.
Then we need to answer the question: how can we **prove** that the result of 
symmetric transfer and %gro are the **only** exceptions from the above rules. 
Or how can we know the list of exceptions wouldn't get longer and longer in the 
future?

Then go back to the example in the summary. From my point of view, the key 
problem is that our escape analysis isn't powerful enough. I don't ask us to do 
excellent escape analysis. It may beyond our abilities. I just want to say how 
can we know the result of symmetric transfer and %gro are the only exceptions.

In D98638#2630778 , @lxfind wrote:

> Whether or not the current coroutine frame would be destroyed completely 
> depend on the implementation of await_suspend. So we cannot predict or know 
> in advance. Therefore, the temporary handle returned by await_suspend must be 
> put on the stack. I don't really see any other solutions other than this.

OK. Although the main stream implementation of await_suspend only destroy the 
coroutine handle in the final awaiter, the compiler can't assume the normal 
await_suspend won't destroy it. So I agree to guard the result of the 
await_suspend to make it put on the stack. At least, it would reduce the size 
of the coroutine frame.

Then if we want to put the result of the await_suspend in the stack, I think we 
can do it under CodeGen part only. It should be easy to judge the return type 
of await_suspend and create a call to llvm.coro.forcestack to the return value 
of await_suspend.

In D98638#2630778 , @lxfind wrote:

> Well, I guess another potential solution is to force emitting lifetime 
> intrinsics for this part of coroutine in the front-end.

I am not sure if this is a good idea. May it break the guide principle in LLVM? 
This need to be reviewed by others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[Xiang Zhang via Phabricator via cfe-commits]

xiangzhangllvm added a comment.

In D98757#2630844 , @LuoYuanke wrote:

> Probably we need a .ll test case to for constant folding.

Fold constant is done in CSE and SCCP which are both passes run in Clang (O2 
)




Comment at: llvm/lib/Analysis/ConstantFolding.cpp:101
 /// Constant fold bitcast, symbolically evaluating it with DataLayout.
 /// This always returns a non-null constant, but it may be a
 /// ConstantExpr if unfoldable.

clin1 wrote:
> API for this function always returns non-null: can we return 
> ConstantExpr::getBitCast(C,DestTy) instead? Then the change in SCCP is not 
> needed either.
I tried, the SCCP will also fold the bitcast into the following instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98757

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

[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[LuoYuanke via Phabricator via cfe-commits]

LuoYuanke added a comment.

> at  clang/test/CodeGen/X86/amx_api.c

Probably we need a .ll test case to for constant folding.




Comment at: llvm/lib/Analysis/ConstantFolding.cpp:108
+  // We won't fold bitcast for tile type, becasue there is no way to
+  // assigne a tmm reg from a constant. We manually generate tilestore
+  // and tileload at pass "Lower AMX type".

assign


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98757

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

Well, I guess another potential solution is to force emitting lifetime 
intrinsics for this part of coroutine in the front-end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[Chang Lin via Phabricator via cfe-commits]

clin1 added inline comments.



Comment at: llvm/lib/Analysis/ConstantFolding.cpp:101
 /// Constant fold bitcast, symbolically evaluating it with DataLayout.
 /// This always returns a non-null constant, but it may be a
 /// ConstantExpr if unfoldable.

API for this function always returns non-null: can we return 
ConstantExpr::getBitCast(C,DestTy) instead? Then the change in SCCP is not 
needed either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98757

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

[PATCH] D98688: [-Wcalled-once-parameter] Harden analysis in terms of block use

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1729
+  handleBlockThatIsGuaranteedToBeCalledOnce(const BlockDecl *Block) override {
+Data.flushWarnings(Block, S);
+  }

Do i understand correctly that you're relying on the order in which your 
analysis is invoked from Sema? I.e., Sema parses the inner block before the 
outer function, so it'll analyze the block first, so by the time you see a 
block expression in the surrounding function you'll already have all 
diagnostics for the block readily available and no other diagnostics will ever 
show up, right?



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2206
 
+sema::AnalysisBasedWarnings::~AnalysisBasedWarnings() { delete IPData; }
+

Do we really need manual new-delete here? Ownership semantics don't seem to be 
that complicated, a `unique_ptr` would probably suffice. Or maybe even just 
store it by value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98688

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

> Here what I want to say is we **shouldn't**  handle all the symmetric 
> transfer from the above analysis. And we shouldn't change the ASTNodes and 
> Sema part. We need to solve about the above pattern. It is not easy to give a 
> solution since user could implement symmetric transfer in final awaiter 
> without destroying the handle, which is more common.

Just to clarify, in case there are any confusions around this. This patch would 
work no matter whether the coroutine frame is destroyed or not during 
await_suspend(). It simply makes sure that the temporary handle returned by 
await_suspend will be put in the stack instead of heap, and it will always be 
safe to do so, no matter what happens.
Whether or not the current coroutine frame would be destroyed completely depend 
on the implementation of await_suspend. So we cannot predict or know in 
advance. Therefore, the temporary handle returned by await_suspend must be put 
on the stack. I don't really see any other solutions other than this.

> It seems to be a workaround to use 
> @llvm.coro.forcestack(%result_of_final_await_suspend) . Since I wondering if 
> there are other corner cases as the %gro. My opinion about 
> '@llvm.coro.forcestack' is that we could use it as a patch if we find any 
> holes that is hard to handle immediately. But we also need to find a solution 
> to solve problems more fundamentally.

Yes as I mentioned in the description, there are really only two cases, one is 
after await_suspend call, and one is gro. gro is easy to handle and I will 
likely send a separate patch latter. But this problem with await_suspend is 
particularly challenging to solve.

What do you think is the fundamental problem, though?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D98694: [-Wcalled-once-parameter] Fix false positives for cleanup attr

[Artem Dergachev via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98694

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

[PATCH] D97916: [Driver][RISCV] Support parsing multi-lib config from GCC.

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng added a comment.

> This still doesn't report that the multilib configuration came from GCC when 
> it succeeds, does it? I suppose that's not a deal-breaker, but it would be 
> nice to have. Would it be difficult to implement?

There is some order issue is those function are executed before printing clang 
version info like `clang version 13.0.0 (g...@github.com:llvm/llvm-project.git 
95cd87efe65d5c485670dd1281bf44dd0bff753d)...`, some script might parse that, so 
I don't like to disrupt that, but I guess I could try to append verbose 
messages into `MultilibErrorMessages` and rename that to 
`MultilibVerboseMessages`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97916

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

[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[Xiang Zhang via Phabricator via cfe-commits]

xiangzhangllvm added a comment.

In D98757#2630764 , @LuoYuanke wrote:

> Would you add a test case for it?

at  clang/test/CodeGen/X86/amx_api.c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98757

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

[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[LuoYuanke via Phabricator via cfe-commits]

LuoYuanke added a comment.

Would you add a test case for it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98757

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

[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[Xiang Zhang via Phabricator via cfe-commits]

xiangzhangllvm added inline comments.



Comment at: clang/test/CodeGen/X86/amx_api.c:39
+void test_tile_init(short row, short col) {
+  __tile1024i c = {row, col, {1, 2, 3}};
+  __tile_stored(buf, STRIDE, c);

we usually write like this __tile1024i c = {row, col};
rm {1,2,3} will also see as {row, col, {0,...}}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98757

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D98638#2630607 , @lxfind wrote:

> In D98638#2628082 , @ChuanqiXu wrote:
>
>> I am a little confused about the first problem. Would it cause the program 
>> to crash? (e.g., we access the fields of coroutine frame after the frame 
>> gets destroyed). Or it just wastes some storage?
>
> This is a repro of the crash (in TSAN mode): https://godbolt.org/z/KvPY66

Oh I got it. The program would crash since handle is destroyed in 
final_awaiter::await_suspend explicitly:

  std::coroutine_handle<> await_suspend(std::coroutine_handle h) 
noexcept {
 h.destroy();
 return std::noop_coroutine();
  }

And the normal symmetric transfer wouldn't destroy the handle (although it 
depends on the implementation of await_suspend).
So the problem met in this patch is program maybe crash with symmetric transfer 
with destroy coroutine handle explicitly (in the final awaiter normally) 
instead of normal symmetric transfer.

The explicitly destruction for the coroutine handle in the await_suspend of 
final awaiter is a normal pattern to enable the Coro-elide optimization. There 
is a discuss before in cafe-dev: 
http://clang-developers.42468.n3.nabble.com/Miscompilation-heap-use-after-free-in-C-coroutines-td4070320.html.
 It looks like it is a subsequent problem.

Here what I want to say is we **shouldn't**  handle all the symmetric transfer 
from the above analysis. And we shouldn't change the ASTNodes and Sema part. We 
need to solve about the above pattern. It is not easy to give a solution since 
user could implement symmetric transfer in final awaiter without destroying the 
handle, which is more common.

My unfinished idea is to emit an intrinsic called @llvm.coro.finalize before we 
emit the promise_type::final_suspend. Then the @llvm.coro.finalize marks the 
end of the lifetime for current coroutine frame. And all the analysis in 
CoroFrame should consider use after @llvm.coro.finalize (We could emit warning 
for some cases). But this idea is also problematic, it makes the semantics of 
coroutine intrinsic more chaos. Just image that how a newbie feels when he see 
@llvm.coro.end, @llvm.coro.destroy and @llvm.coro.finalize. And we can't use 
@llvm.coro.end @llvm.coro.destroy  since they have other semantics 
(llvm.coro.destroy means deletion and llvm.coro.end would be used to split the 
coroutine). Also, the idea of @llvm.coro.finalize seems available to solve the 
problem about%gro mentioned above.

It seems to be a workaround to use 
@llvm.coro.forcestack(%result_of_final_await_suspend) . Since I wondering if 
there are other corner cases as the %gro. My opinion about 
'@llvm.coro.forcestack' is that we could use it as a patch if we find any holes 
that is hard to handle immediately. But we also need to find a solution to 
solve problems more fundamentally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D98685: [X86][AMX] Rename amx-bf16 intrinsic according to correct naming convention

[Bing Yu 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 rG320b72e9cd77: [X86][AMX] Rename amx-bf16 intrinsic according 
to correct naming convention (authored by yubing).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98685

Files:
  clang/lib/Headers/amxintrin.h
  clang/test/CodeGen/X86/amx_api.c


Index: clang/test/CodeGen/X86/amx_api.c
===
--- clang/test/CodeGen/X86/amx_api.c
+++ clang/test/CodeGen/X86/amx_api.c
@@ -81,9 +81,9 @@
   __tile_zero();
 }
 
-void test_tile_tdpbf16ps(__tile1024i a, __tile1024i b, __tile1024i c) {
-  //CHECK-LABEL: @test_tile_tdpbf16ps
+void test_tile_dpbf16ps(__tile1024i a, __tile1024i b, __tile1024i c) {
+  //CHECK-LABEL: @test_tile_dpbf16ps
   //CHECK: call x86_amx @llvm.x86.tdpbf16ps.internal
   //CHECK-NEXT: {{%.*}} = bitcast x86_amx {{%.*}} to <256 x i32>
-  __tile_tdpbf16ps(, b, c);
+  __tile_dpbf16ps(, b, c);
 }
Index: clang/lib/Headers/amxintrin.h
===
--- clang/lib/Headers/amxintrin.h
+++ clang/lib/Headers/amxintrin.h
@@ -267,8 +267,8 @@
 }
 
 static __inline__ _tile1024i __DEFAULT_FN_ATTRS_BF16
-_tile_tdpbf16ps_internal(unsigned short m, unsigned short n, unsigned short k,
- _tile1024i dst, _tile1024i src1, _tile1024i src2) {
+_tile_dpbf16ps_internal(unsigned short m, unsigned short n, unsigned short k,
+_tile1024i dst, _tile1024i src1, _tile1024i src2) {
   return __builtin_ia32_tdpbf16ps_internal(m, n, k, dst, src1, src2);
 }
 
@@ -323,10 +323,10 @@
 }
 
 __DEFAULT_FN_ATTRS_BF16
-static void __tile_tdpbf16ps(__tile1024i *dst, __tile1024i src1,
- __tile1024i src2) {
-  dst->tile = _tile_tdpbf16ps_internal(src1.row, src2.col, src1.col, dst->tile,
-   src1.tile, src2.tile);
+static void __tile_dpbf16ps(__tile1024i *dst, __tile1024i src1,
+__tile1024i src2) {
+  dst->tile = _tile_dpbf16ps_internal(src1.row, src2.col, src1.col, dst->tile,
+  src1.tile, src2.tile);
 }
 
 #undef __DEFAULT_FN_ATTRS_TILE


Index: clang/test/CodeGen/X86/amx_api.c
===
--- clang/test/CodeGen/X86/amx_api.c
+++ clang/test/CodeGen/X86/amx_api.c
@@ -81,9 +81,9 @@
   __tile_zero();
 }
 
-void test_tile_tdpbf16ps(__tile1024i a, __tile1024i b, __tile1024i c) {
-  //CHECK-LABEL: @test_tile_tdpbf16ps
+void test_tile_dpbf16ps(__tile1024i a, __tile1024i b, __tile1024i c) {
+  //CHECK-LABEL: @test_tile_dpbf16ps
   //CHECK: call x86_amx @llvm.x86.tdpbf16ps.internal
   //CHECK-NEXT: {{%.*}} = bitcast x86_amx {{%.*}} to <256 x i32>
-  __tile_tdpbf16ps(, b, c);
+  __tile_dpbf16ps(, b, c);
 }
Index: clang/lib/Headers/amxintrin.h
===
--- clang/lib/Headers/amxintrin.h
+++ clang/lib/Headers/amxintrin.h
@@ -267,8 +267,8 @@
 }
 
 static __inline__ _tile1024i __DEFAULT_FN_ATTRS_BF16
-_tile_tdpbf16ps_internal(unsigned short m, unsigned short n, unsigned short k,
- _tile1024i dst, _tile1024i src1, _tile1024i src2) {
+_tile_dpbf16ps_internal(unsigned short m, unsigned short n, unsigned short k,
+_tile1024i dst, _tile1024i src1, _tile1024i src2) {
   return __builtin_ia32_tdpbf16ps_internal(m, n, k, dst, src1, src2);
 }
 
@@ -323,10 +323,10 @@
 }
 
 __DEFAULT_FN_ATTRS_BF16
-static void __tile_tdpbf16ps(__tile1024i *dst, __tile1024i src1,
- __tile1024i src2) {
-  dst->tile = _tile_tdpbf16ps_internal(src1.row, src2.col, src1.col, dst->tile,
-   src1.tile, src2.tile);
+static void __tile_dpbf16ps(__tile1024i *dst, __tile1024i src1,
+__tile1024i src2) {
+  dst->tile = _tile_dpbf16ps_internal(src1.row, src2.col, src1.col, dst->tile,
+  src1.tile, src2.tile);
 }
 
 #undef __DEFAULT_FN_ATTRS_TILE
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 320b72e - [X86][AMX] Rename amx-bf16 intrinsic according to correct naming convention

[Bing1 Yu via cfe-commits]

Author: Bing1 Yu
Date: 2021-03-17T11:22:52+08:00
New Revision: 320b72e9cd77504054bd2c837149df2f2bd4c149

URL: 
https://github.com/llvm/llvm-project/commit/320b72e9cd77504054bd2c837149df2f2bd4c149
DIFF: 
https://github.com/llvm/llvm-project/commit/320b72e9cd77504054bd2c837149df2f2bd4c149.diff

LOG: [X86][AMX] Rename amx-bf16 intrinsic according to correct naming convention

__tile_tdpbf16ps should be renamed with __tile_dpbf16ps

Reviewed By: pengfei

Differential Revision: https://reviews.llvm.org/D98685

Added: 


Modified: 
clang/lib/Headers/amxintrin.h
clang/test/CodeGen/X86/amx_api.c

Removed: 




diff  --git a/clang/lib/Headers/amxintrin.h b/clang/lib/Headers/amxintrin.h
index 8c276519e362..12d21d40bcff 100644
--- a/clang/lib/Headers/amxintrin.h
+++ b/clang/lib/Headers/amxintrin.h
@@ -267,8 +267,8 @@ _tile_stored_internal(unsigned short m, unsigned short n, 
void *base,
 }
 
 static __inline__ _tile1024i __DEFAULT_FN_ATTRS_BF16
-_tile_tdpbf16ps_internal(unsigned short m, unsigned short n, unsigned short k,
- _tile1024i dst, _tile1024i src1, _tile1024i src2) {
+_tile_dpbf16ps_internal(unsigned short m, unsigned short n, unsigned short k,
+_tile1024i dst, _tile1024i src1, _tile1024i src2) {
   return __builtin_ia32_tdpbf16ps_internal(m, n, k, dst, src1, src2);
 }
 
@@ -323,10 +323,10 @@ static void __tile_zero(__tile1024i *dst) {
 }
 
 __DEFAULT_FN_ATTRS_BF16
-static void __tile_tdpbf16ps(__tile1024i *dst, __tile1024i src1,
- __tile1024i src2) {
-  dst->tile = _tile_tdpbf16ps_internal(src1.row, src2.col, src1.col, dst->tile,
-   src1.tile, src2.tile);
+static void __tile_dpbf16ps(__tile1024i *dst, __tile1024i src1,
+__tile1024i src2) {
+  dst->tile = _tile_dpbf16ps_internal(src1.row, src2.col, src1.col, dst->tile,
+  src1.tile, src2.tile);
 }
 
 #undef __DEFAULT_FN_ATTRS_TILE

diff  --git a/clang/test/CodeGen/X86/amx_api.c 
b/clang/test/CodeGen/X86/amx_api.c
index 824a3aec20ec..3bfe887c0445 100644
--- a/clang/test/CodeGen/X86/amx_api.c
+++ b/clang/test/CodeGen/X86/amx_api.c
@@ -81,9 +81,9 @@ void test_tile_zero(__tile1024i c) {
   __tile_zero();
 }
 
-void test_tile_tdpbf16ps(__tile1024i a, __tile1024i b, __tile1024i c) {
-  //CHECK-LABEL: @test_tile_tdpbf16ps
+void test_tile_dpbf16ps(__tile1024i a, __tile1024i b, __tile1024i c) {
+  //CHECK-LABEL: @test_tile_dpbf16ps
   //CHECK: call x86_amx @llvm.x86.tdpbf16ps.internal
   //CHECK-NEXT: {{%.*}} = bitcast x86_amx {{%.*}} to <256 x i32>
-  __tile_tdpbf16ps(, b, c);
+  __tile_dpbf16ps(, b, c);
 }



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

[PATCH] D98757: [AMX] Not fold constant bitcast into amx intrisic

[Xiang Zhang via Phabricator via cfe-commits]

xiangzhangllvm created this revision.
xiangzhangllvm added reviewers: LuoYuanke, pengfei, LiuChen3, yubing.
Herald added a subscriber: hiraditya.
xiangzhangllvm requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

We won't fold bitcast for tile type, becasue there is no way to
assignee a tmm reg from a constant. We manually generate tilestore
and tileload at pass "Lower AMX type".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98757

Files:
  clang/test/CodeGen/X86/amx_api.c
  llvm/lib/Analysis/ConstantFolding.cpp
  llvm/lib/Transforms/Scalar/SCCP.cpp


Index: llvm/lib/Transforms/Scalar/SCCP.cpp
===
--- llvm/lib/Transforms/Scalar/SCCP.cpp
+++ llvm/lib/Transforms/Scalar/SCCP.cpp
@@ -826,7 +826,7 @@
   if (Constant *OpC = getConstant(OpSt)) {
 // Fold the constant as we build.
 Constant *C = ConstantFoldCastOperand(I.getOpcode(), OpC, I.getType(), DL);
-if (isa(C))
+if (!C || isa(C))
   return;
 // Propagate constant value
 markConstant(, C);
Index: llvm/lib/Analysis/ConstantFolding.cpp
===
--- llvm/lib/Analysis/ConstantFolding.cpp
+++ llvm/lib/Analysis/ConstantFolding.cpp
@@ -104,10 +104,16 @@
   assert(CastInst::castIsValid(Instruction::BitCast, C, DestTy) &&
  "Invalid constantexpr bitcast!");
 
+  // We won't fold bitcast for tile type, becasue there is no way to
+  // assigne a tmm reg from a constant. We manually generate tilestore
+  // and tileload at pass "Lower AMX type".
+  if (DestTy->isX86_AMXTy())
+return nullptr;
+
   // Catch the obvious splat cases.
-  if (C->isNullValue() && !DestTy->isX86_MMXTy() && !DestTy->isX86_AMXTy())
+  if (C->isNullValue() && !DestTy->isX86_MMXTy())
 return Constant::getNullValue(DestTy);
-  if (C->isAllOnesValue() && !DestTy->isX86_MMXTy() && !DestTy->isX86_AMXTy() 
&&
+  if (C->isAllOnesValue() && !DestTy->isX86_MMXTy() &&
   !DestTy->isPtrOrPtrVectorTy()) // Don't get ones for ptr types!
 return Constant::getAllOnesValue(DestTy);
 
Index: clang/test/CodeGen/X86/amx_api.c
===
--- clang/test/CodeGen/X86/amx_api.c
+++ clang/test/CodeGen/X86/amx_api.c
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 %s -flax-vector-conversions=none -ffreestanding 
-triple=x86_64-unknown-unknown  -target-feature +avx512f  -target-feature 
+amx-int8  \
 // RUN: -target-feature +amx-bf16 -emit-llvm -o - -Werror -pedantic | 
FileCheck %s --check-prefixes=CHECK
 
+// RUN: %clang_cc1 %s -flax-vector-conversions=none -ffreestanding 
-triple=x86_64-unknown-unknown  -target-feature +avx512f  -target-feature 
+amx-int8  \
+// RUN: -target-feature +amx-bf16 -O2 -emit-llvm -o - -Werror -pedantic | 
FileCheck %s --check-prefixes=CHECK2
+
 #include 
 
 char buf[1024];
@@ -31,6 +34,15 @@
   __tile_stored(buf, STRIDE, c);
 }
 
+// Not fold the bitcast const vector into amx intrisic.
+void test_tile_init(short row, short col) {
+  __tile1024i c = {row, col, {1, 2, 3}};
+  __tile_stored(buf, STRIDE, c);
+  //CHECK2-LABEL: @test_tile_init
+  //CHECK2: {{%.*}} = bitcast <256 x i32> Index: llvm/lib/Transforms/Scalar/SCCP.cpp
===
--- llvm/lib/Transforms/Scalar/SCCP.cpp
+++ llvm/lib/Transforms/Scalar/SCCP.cpp
@@ -826,7 +826,7 @@
   if (Constant *OpC = getConstant(OpSt)) {
 // Fold the constant as we build.
 Constant *C = ConstantFoldCastOperand(I.getOpcode(), OpC, I.getType(), DL);
-if (isa(C))
+if (!C || isa(C))
   return;
 // Propagate constant value
 markConstant(, C);
Index: llvm/lib/Analysis/ConstantFolding.cpp
===
--- llvm/lib/Analysis/ConstantFolding.cpp
+++ llvm/lib/Analysis/ConstantFolding.cpp
@@ -104,10 +104,16 @@
   assert(CastInst::castIsValid(Instruction::BitCast, C, DestTy) &&
  "Invalid constantexpr bitcast!");
 
+  // We won't fold bitcast for tile type, becasue there is no way to
+  // assigne a tmm reg from a constant. We manually generate tilestore
+  // and tileload at pass "Lower AMX type".
+  if (DestTy->isX86_AMXTy())
+return nullptr;
+
   // Catch the obvious splat cases.
-  if (C->isNullValue() && !DestTy->isX86_MMXTy() && !DestTy->isX86_AMXTy())
+  if (C->isNullValue() && !DestTy->isX86_MMXTy())
 return Constant::getNullValue(DestTy);
-  if (C->isAllOnesValue() && !DestTy->isX86_MMXTy() && !DestTy->isX86_AMXTy() &&
+  if (C->isAllOnesValue() && !DestTy->isX86_MMXTy() &&
   !DestTy->isPtrOrPtrVectorTy()) // Don't get ones for ptr types!
 return Constant::getAllOnesValue(DestTy);
 
Index: clang/test/CodeGen/X86/amx_api.c
===
--- clang/test/CodeGen/X86/amx_api.c
+++ clang/test/CodeGen/X86/amx_api.c
@@ -1,6 +1,9 @@
 // RUN: 

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Tony Tye via Phabricator via cfe-commits]

t-tye added a comment.

I vaguely remember that clang needed to know what code object it was going to 
request as it used that to either validate other options, or change the format 
of other passed cc1 options. If that is true, then I am not sure the defaulting 
approach works as clang will not know what the backend will be defaulting to. 
@yaxunl can you remember this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

[Ally Tiritoglu via Phabricator via cfe-commits]

atirit added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:3683
+Style.BraceWrapping.AfterEnum;
+bool isLineTooBig = (strlen(Right.TokenText.data()) +
+ Right.OriginalColumn) > Style.ColumnLimit;

curdeius wrote:
> atirit wrote:
> > curdeius wrote:
> > > strlen? Please use `StringRef::size()`.
> > For `FormatToken.TokenText`, `StringRef::size()` is set to the length of 
> > the token, not of the stored text. Please see 
> > `clang/lib/Format/FormatTokenLexer.cpp:1047`. Changing that line breaks 
> > nearly every format test so I'm assuming that it is correct. A `strlen` or 
> > equivalent is necessary here.
> Then it should be something like the line's length, no? Using `strlen` will 
> be very expensive on non-snippets, as it `strlen` will traverse the string 
> until its end (so possibly until the end of file) for each invocation of 
> `mustBreakBefore` (if it goes into this condition of course).
> 
> I only see one failing check in the test `FormatTest.FormatsTypedefEnum` when 
> using `TokenText.size()`:
> ```
>   verifyFormat("typedef enum\n"
>"{\n"
>"  ZERO = 0,\n"
>"  ONE = 1,\n"
>"  TWO = 2,\n"
>"  THREE = 3\n"
>"} LongEnum;",
>Style);
> ```
> 
> You might need to add more tests in `AfterEnum` to test the behaviour of this 
> part if the line is just below/above the limit.
> 
> Also, that's just a hack, but I made all tests pass with:
> ```
> assert(Line.Last);
> assert(Line.Last->TokenText.data() >= Right.TokenText.data());
> auto isAllowedByShortEnums = [&]() {
>   if (!Style.AllowShortEnumsOnASingleLine ||
>   (Line.Last->TokenText.data() - Right.TokenText.data() +
>Right.TokenText.size() + Right.OriginalColumn) >
>   Style.ColumnLimit)
> ```
> I haven't given it too much thought though and am unsure whether there are 
> cases where the above assertions will fail.
If I use `TokenText.size()`, the line length check will always claim that the 
line is short enough. I'll be adding a unit test for this. However, you're 
right that a `strlen` is a bad idea here. I hadn't realised it would consume 
the entire file. I'll try to figure out a more efficient method of checking the 
line length.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93938

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124
+  // Currently defaults to 3 in AMDGPUBaseInfo.cpp
+  // Using that default lets clang emit IR for amdgcn when llvm has been built
+  // without that target, provided the user wants this code object version
+  if (CodeObjVer != 3) {
+CmdArgs.insert(CmdArgs.begin() + 1,
+   Args.MakeArgString(Twine("--amdhsa-code-object-version=") +
+  Twine(CodeObjVer)));

yaxunl wrote:
> jdoerfert wrote:
> > JonChesterfield wrote:
> > > t-tye wrote:
> > > > This seem rather fragile. If the backend default changes and this code 
> > > > is not updated it will likely result in creating the wrong code object 
> > > > version.
> > > That is true, though numbers 2 through 4 are used in 
> > > getOrCheckAMDGPUCodeObjectVersion with 3 as the default there too.
> > > 
> > > Equally, today if we change the default in clang and forget to update the 
> > > backend, we won't notice because we always pass this argument.
> > > 
> > > What we have at present is the back end has the state and the front end 
> > > chooses the default. Needing to write '3' in both places is a symptom of 
> > > that. Perhaps the back end should not have a default for it, requiring 
> > > this to always be passed, at which point we are going to have a bad time 
> > > building amdgpu openmp by default.
> > > 
> > > This change decouples the two for the (likely common) case where the user 
> > > has not chosen to override the value. It's not ideal, but equally we 
> > > don't change the version that often and there are lit tests that notice 
> > > if we get it wrong. Plus stuff seems to break if the code object version 
> > > is not as expected.
> > > 
> > > There may be a better solution. Target specific variable set by clang 
> > > feels like something that will occur elsewhere.
> > Arguably, the backend should have a default, the option should be used by 
> > users that don't like the default.
> > 
> I think a reasonable behavior of clang is to add -mllvm 
> --amdhsa-code-object-version=x only if users use -mcode-object-version=x or 
> equivalent explicitly. Otherwise clang should not add -mllvm 
> --amdhsa-code-object-version=x and let the backend decides the default code 
> object version.
> 
> @t-tye @kzhuravl what do you think? thanks.
That sounds right to me. It's slightly more complicated to implement than this, 
will need to rework getOrCheckAMDGPUCodeObjectVersion to distinguish between 
the get and check parts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D96033#2630585 , @rsmith wrote:

> @rjmccall I'd like your input on this patch in particular, if you have time.

I've been specifically avoiding paying any attention to this patch because it 
sounds like an enormous time-sink to review. :)  That's not to say it'd be time 
poorly spent, because it's an intriguing feature, but I just don't have the 
time to engage with it fully.  I can try to give snippets of time if you can 
pose specific questions.  I did at least go back and read the RFC from the 
summer.  I'm not sure I have time to read the review thread up until now; it's 
quite daunting.

> I'm nervous in general about the looming idea of declaration unloading, but 
> the fact that it's been working in Cling for a long time gives me some 
> confidence that we can do that in a way that's not prohibitively expensive 
> and invasive.

I don't really know the jargon here.  The biggest problem that I foresee around 
having a full-featured C REPL is the impossibility of replacing existing types 
— you might be able to introduce a *new* struct with a particular name, break 
the redeclaration chain, and have it shadow the old one, and we could probably 
chase down all the engineering problems that that causes in the compiler, but 
it's never going to be a particularly satisfying model.  If we don't have to 
worry about that, then I feel like the largest problem is probably the IRGen 
interaction — in particular, whether we're going to have to start serializing 
IRGen state the same way that Sema state has to be serialized across PCH 
boundaries.  But I'm sure the people who are working on this have more 
knowledge of what issues they're seeing than I can just abstractly anticipate.


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

https://reviews.llvm.org/D96033

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

[PATCH] D98676: [WebAssembly] Finalize SIMD names and opcodes

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.



Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:1243
 
 // Prototype f64x2 conversions
 defm "" : SIMDConvert If they are finalized, can we delete these "prototype" comments here and 
> elsewhere? (not necessarily in this CL)
Yes, this definitely makes sense. I think I will leave it for future patches 
that also replace the intrinsics with proper patterns where applicable, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98676

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

[clang] f5030f1 - [AST] Suppress diagnostic output when generating code

[Stephen Kelly via cfe-commits]

Author: Stephen Kelly
Date: 2021-03-17T01:30:22Z
New Revision: f5030f1a8e4affef2ab92b3268292f46d0052fd5

URL: 
https://github.com/llvm/llvm-project/commit/f5030f1a8e4affef2ab92b3268292f46d0052fd5
DIFF: 
https://github.com/llvm/llvm-project/commit/f5030f1a8e4affef2ab92b3268292f46d0052fd5.diff

LOG: [AST] Suppress diagnostic output when generating code

Added: 


Modified: 
clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp

Removed: 




diff  --git a/clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp 
b/clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp
index 74ba70eefa04..06b58c6382ed 100644
--- a/clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp
+++ b/clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp
@@ -135,6 +135,8 @@ int main(int argc, const char **argv) {
   if (!Compiler.hasDiagnostics())
 return 1;
 
+  // Suppress "2 errors generated" or similar messages
+  Compiler.getDiagnosticOpts().ShowCarets = false;
   Compiler.createSourceManager(Files);
 
   ASTSrcLocGenerationAction ScopedToolAction;



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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 2 inline comments as done.
gulfem added inline comments.



Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391
+
+if (!TM.getTargetTriple().isArch64Bit())
+  return false;
+

lebedev.ri wrote:
> 1. But all tests are using `x86_64` triple?
> 2. This is somewhat backwards. if the target wants to disable this, it will 
> need to override this function with `return false;`.
1. Although I used `x86_64 triple`, this optimization can be applied to other 
64-bit architectures too, because it not target dependent except `isArch64Bit` 
and `getCodeModel` check.
2. Is there a target that you have in mind that we need to disable this 
optimization? 
I thought that it makes sense to enable this optimization by default on all the 
targets that can support it.
In case targets want to disable it, they can override it as you said.
How can we improve the implementation?
If you have suggestions, I'm happy to incorporate that.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 4 inline comments as done.
gulfem added inline comments.



Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:32
+  // If lookup table has more than one user,
+  // do not generate a relative lookup table.
+  if (!GV.hasOneUse())

hans wrote:
> It would be better if the comment said why. I suppose the reason is we need 
> to be sure there aren't other uses of the table, because then it can't be 
> replaced.
> 
> But it would be cool if a future version of this patch could handle when 
> there are multiple loads from the table which can all be replaced -- for 
> example this could happen if a function which uses a lookup table gets 
> inlined into multiple places.
I actually ran into the exact case that you described during testing, where a 
function that uses a switch gets inlined into multiple call sites :)
This is only to simplify the analysis, and I now added a TODO section to 
explore that later.



Comment at: 
llvm/test/Transforms/RelLookupTableConverter/switch_relative_lookup_table.ll:39
+; ; Relative switch lookup table for strings
+define i8* @string_table(i32 %cond) {
+  ; FNOPIC-LABEL: @string_table(

leonardchan wrote:
> It looks like this test case isn't much different from `string_table` in 
> `relative_lookup_table.ll`? If so, then this file could be removed.
I renamed this test case to no_relative_lookup_table.ll that checks the cases 
where relative lookup table should not be generated like in non-pic mode, 
medium or large code models, and 32 bit architectures, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124
+  // Currently defaults to 3 in AMDGPUBaseInfo.cpp
+  // Using that default lets clang emit IR for amdgcn when llvm has been built
+  // without that target, provided the user wants this code object version
+  if (CodeObjVer != 3) {
+CmdArgs.insert(CmdArgs.begin() + 1,
+   Args.MakeArgString(Twine("--amdhsa-code-object-version=") +
+  Twine(CodeObjVer)));

jdoerfert wrote:
> JonChesterfield wrote:
> > t-tye wrote:
> > > This seem rather fragile. If the backend default changes and this code is 
> > > not updated it will likely result in creating the wrong code object 
> > > version.
> > That is true, though numbers 2 through 4 are used in 
> > getOrCheckAMDGPUCodeObjectVersion with 3 as the default there too.
> > 
> > Equally, today if we change the default in clang and forget to update the 
> > backend, we won't notice because we always pass this argument.
> > 
> > What we have at present is the back end has the state and the front end 
> > chooses the default. Needing to write '3' in both places is a symptom of 
> > that. Perhaps the back end should not have a default for it, requiring this 
> > to always be passed, at which point we are going to have a bad time 
> > building amdgpu openmp by default.
> > 
> > This change decouples the two for the (likely common) case where the user 
> > has not chosen to override the value. It's not ideal, but equally we don't 
> > change the version that often and there are lit tests that notice if we get 
> > it wrong. Plus stuff seems to break if the code object version is not as 
> > expected.
> > 
> > There may be a better solution. Target specific variable set by clang feels 
> > like something that will occur elsewhere.
> Arguably, the backend should have a default, the option should be used by 
> users that don't like the default.
> 
I think a reasonable behavior of clang is to add -mllvm 
--amdhsa-code-object-version=x only if users use -mcode-object-version=x or 
equivalent explicitly. Otherwise clang should not add -mllvm 
--amdhsa-code-object-version=x and let the backend decides the default code 
object version.

@t-tye @kzhuravl what do you think? thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 331139.
gulfem added a comment.

1. Add no_relative_lookup_table.ll test case that checks the cases where 
relative lookup table should not be generated
2. Add comments about dso_local check and single use check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  llvm/docs/Passes.rst
  llvm/include/llvm/Analysis/TargetTransformInfo.h
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/CodeGen/BasicTTIImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Utils/RelLookupTableConverter.h
  llvm/lib/Analysis/TargetTransformInfo.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/opt-O2-pipeline.ll
  llvm/test/Other/opt-O3-pipeline-enable-matrix.ll
  llvm/test/Other/opt-O3-pipeline.ll
  llvm/test/Other/opt-Os-pipeline.ll
  llvm/test/Other/pass-pipelines.ll
  llvm/test/Transforms/RelLookupTableConverter/X86/no_relative_lookup_table.ll
  llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
  llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
@@ -61,6 +61,7 @@
 "NameAnonGlobals.cpp",
 "PredicateInfo.cpp",
 "PromoteMemoryToRegister.cpp",
+"RelLookupTableConverter.cpp"
 "SSAUpdater.cpp",
 "SSAUpdaterBulk.cpp",
 "SampleProfileLoaderBaseUtil.cpp",
Index: llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
@@ -0,0 +1,310 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+; RUN: opt < %s -passes=rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+@.str.4 = private unnamed_addr constant [6 x i8] c"three\00", align 1
+@.str.5 = private unnamed_addr constant [5 x i8] c"str1\00", align 1
+@.str.6 = private unnamed_addr constant [5 x i8] c"str2\00", align 1
+@.str.7 = private unnamed_addr constant [12 x i8] c"singlevalue\00", align 1
+
+@a1 = external global i32, align 4
+@b1 = external global i32, align 4
+@c1 = external global i32, align 4
+@d1 = external global i32, align 4
+
+@a2 = internal global i32 0, align 4
+@b2 = internal global i32 0, align 4
+@c2 = internal global i32 0, align 4
+@d2 = internal global i32 0, align 4
+
+@hidden0 = external hidden global i32, align 8
+@hidden1 = external hidden global i32, align 8
+@hidden2 = external hidden global i32, align 8
+@hidden3 = external hidden global i32, align 8
+
+@switch.table.no_dso_local = private unnamed_addr constant [3 x i32*] [i32* @a1, i32* @b1, i32* @c1], align 8
+
+@switch.table.dso_local = private unnamed_addr constant [3 x i32*] [i32* @a2, i32* @b2, i32* @c2], align 8
+
+@switch.table.hidden = private unnamed_addr constant [3 x i32*] [i32* @hidden0, i32* @hidden1, i32* @hidden2], align 8
+
+@switch.table.string_table = private unnamed_addr constant [3 x i8*]
+ [
+  i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+ ], align 8
+
+@switch.table.string_table_holes = private unnamed_addr constant [4 x i8*]
+   [
+i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+   

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Xun Li via Phabricator via cfe-commits]

lxfind added inline comments.



Comment at: clang/include/clang/AST/ExprCXX.h:4695
+/// afterwards on the stack.
 class CoroutineSuspendExpr : public Expr {
   friend class ASTStmtReader;

lxfind wrote:
> ChuanqiXu wrote:
> > It looks strange for the change of `CoroutineSuspendExpr` at the first 
> > glance. It is easy to understand the coroutine suspend expression is 
> > consists of three parts: Ready, Suspend and resume. It is written in the 
> > language documentation. And the new added AwaitSuspendCall is confusing.
> I agree. But this seems to be the only way to break up Suspend at the point 
> of await_suspend call so that we can insert instructions during CodeGen. Open 
> to ideas though.
One potential way to make this more clear is to rename these two nodes as: 
Suspend and Transfer.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

In D98638#2628082 , @ChuanqiXu wrote:

> I am a little confused about the first problem. Would it cause the program to 
> crash? (e.g., we access the fields of coroutine frame after the frame gets 
> destroyed). Or it just wastes some storage?

This is a repro of the crash (in TSAN mode): https://godbolt.org/z/KvPY66


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

@rjmccall I'd like your input on this patch in particular, if you have time.

I'm nervous in general about the looming idea of declaration unloading, but the 
fact that it's been working in Cling for a long time gives me some confidence 
that we can do that in a way that's not prohibitively expensive and invasive.


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

https://reviews.llvm.org/D96033

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

[PATCH] D98676: [WebAssembly] Finalize SIMD names and opcodes

[Heejin Ahn via Phabricator via cfe-commits]

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



Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:1243
 
 // Prototype f64x2 conversions
 defm "" : SIMDConverthttps://reviews.llvm.org/D98676/new/

https://reviews.llvm.org/D98676

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Tony Tye via Phabricator via cfe-commits]

t-tye added a comment.

I have no opinion, just making an observation and defer to @kzhuravl .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D97358: [X86] Support amx-bf16 intrinsic.

[Xiang Zhang via Phabricator via cfe-commits]

xiangzhangllvm added a comment.

+1 first, didn't see key problems.




Comment at: clang/lib/Headers/amxintrin.h:326
+__DEFAULT_FN_ATTRS_BF16
+static void __tile_tdpbf16ps(__tile1024i *dst, __tile1024i src1,
+ __tile1024i src2) {

yubing wrote:
> Should we align this with "tile_dpbssd" by renaming it wth "tile_dpbf16ps"?
Yes, "t" already means "tile"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97358

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124
+  // Currently defaults to 3 in AMDGPUBaseInfo.cpp
+  // Using that default lets clang emit IR for amdgcn when llvm has been built
+  // without that target, provided the user wants this code object version
+  if (CodeObjVer != 3) {
+CmdArgs.insert(CmdArgs.begin() + 1,
+   Args.MakeArgString(Twine("--amdhsa-code-object-version=") +
+  Twine(CodeObjVer)));

JonChesterfield wrote:
> t-tye wrote:
> > This seem rather fragile. If the backend default changes and this code is 
> > not updated it will likely result in creating the wrong code object version.
> That is true, though numbers 2 through 4 are used in 
> getOrCheckAMDGPUCodeObjectVersion with 3 as the default there too.
> 
> Equally, today if we change the default in clang and forget to update the 
> backend, we won't notice because we always pass this argument.
> 
> What we have at present is the back end has the state and the front end 
> chooses the default. Needing to write '3' in both places is a symptom of 
> that. Perhaps the back end should not have a default for it, requiring this 
> to always be passed, at which point we are going to have a bad time building 
> amdgpu openmp by default.
> 
> This change decouples the two for the (likely common) case where the user has 
> not chosen to override the value. It's not ideal, but equally we don't change 
> the version that often and there are lit tests that notice if we get it 
> wrong. Plus stuff seems to break if the code object version is not as 
> expected.
> 
> There may be a better solution. Target specific variable set by clang feels 
> like something that will occur elsewhere.
Arguably, the backend should have a default, the option should be used by users 
that don't like the default.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124
+  // Currently defaults to 3 in AMDGPUBaseInfo.cpp
+  // Using that default lets clang emit IR for amdgcn when llvm has been built
+  // without that target, provided the user wants this code object version
+  if (CodeObjVer != 3) {
+CmdArgs.insert(CmdArgs.begin() + 1,
+   Args.MakeArgString(Twine("--amdhsa-code-object-version=") +
+  Twine(CodeObjVer)));

t-tye wrote:
> This seem rather fragile. If the backend default changes and this code is not 
> updated it will likely result in creating the wrong code object version.
That is true, though numbers 2 through 4 are used in 
getOrCheckAMDGPUCodeObjectVersion with 3 as the default there too.

Equally, today if we change the default in clang and forget to update the 
backend, we won't notice because we always pass this argument.

What we have at present is the back end has the state and the front end chooses 
the default. Needing to write '3' in both places is a symptom of that. Perhaps 
the back end should not have a default for it, requiring this to always be 
passed, at which point we are going to have a bad time building amdgpu openmp 
by default.

This change decouples the two for the (likely common) case where the user has 
not chosen to override the value. It's not ideal, but equally we don't change 
the version that often and there are lit tests that notice if we get it wrong. 
Plus stuff seems to break if the code object version is not as expected.

There may be a better solution. Target specific variable set by clang feels 
like something that will occur elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D93164: [AST] Add generator for source location introspection

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

In D93164#2630534 , @steveire wrote:

> In D93164#2630203 , @nathanchance 
> wrote:
>
>> I am seeing a spew of errors after 19740652c4c4329e2b9e77f96e5e31c360b4e8bb 
>>  (what 
>> appears to be the latest version of this patch):
>
> Hi @nathanchance Does it make your build fail? I have pushed a fix. Can you 
> update and try again?

Does not look like my build ever errored out (exit code was 0 even without the 
fix you pushed).

It does look like the errors are hidden now though (mostly but that is fine 
enough for me):

  [1373/1373] ASTNodeAPI.json
  20 errors generated.

Thanks for the quick response!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93164

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124
+  // Currently defaults to 3 in AMDGPUBaseInfo.cpp
+  // Using that default lets clang emit IR for amdgcn when llvm has been built
+  // without that target, provided the user wants this code object version
+  if (CodeObjVer != 3) {
+CmdArgs.insert(CmdArgs.begin() + 1,
+   Args.MakeArgString(Twine("--amdhsa-code-object-version=") +
+  Twine(CodeObjVer)));

This seem rather fragile. If the backend default changes and this code is not 
updated it will likely result in creating the wrong code object version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D93164: [AST] Add generator for source location introspection

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D93164#2630203 , @nathanchance 
wrote:

> I am seeing a spew of errors after 19740652c4c4329e2b9e77f96e5e31c360b4e8bb 
>  (what 
> appears to be the latest version of this patch):

Hi @nathanchance Does it make your build fail? I have pushed a fix. Can you 
update and try again?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93164

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

[clang] a00d440 - [AST] Hide errors from the attempt to introspect nodes

[Stephen Kelly via cfe-commits]

Author: Stephen Kelly
Date: 2021-03-16T23:46:31Z
New Revision: a00d44012820e9ed2eba623dd61ca9cf5a2ce115

URL: 
https://github.com/llvm/llvm-project/commit/a00d44012820e9ed2eba623dd61ca9cf5a2ce115
DIFF: 
https://github.com/llvm/llvm-project/commit/a00d44012820e9ed2eba623dd61ca9cf5a2ce115.diff

LOG: [AST] Hide errors from the attempt to introspect nodes

Added: 


Modified: 
clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp

Removed: 




diff  --git a/clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp 
b/clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp
index 6615f865221d..74ba70eefa04 100644
--- a/clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp
+++ b/clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp
@@ -92,7 +92,13 @@ int main(int argc, const char **argv) {
   auto ParsedArgs = Opts.ParseArgs(llvm::makeArrayRef(Argv).slice(1),
MissingArgIndex, MissingArgCount);
   ParseDiagnosticArgs(*DiagOpts, ParsedArgs);
-  TextDiagnosticPrinter DiagnosticPrinter(llvm::errs(), &*DiagOpts);
+
+  // Don't output diagnostics, because common scenarios such as
+  // cross-compiling fail with diagnostics.  This is not fatal, but
+  // just causes attempts to use the introspection API to return no data.
+  std::string Str;
+  llvm::raw_string_ostream OS(Str);
+  TextDiagnosticPrinter DiagnosticPrinter(OS, &*DiagOpts);
   DiagnosticsEngine Diagnostics(
   IntrusiveRefCntPtr(new DiagnosticIDs()), &*DiagOpts,
   , false);



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

[PATCH] D98296: [clang-tidy] Simplify readability checks to not need ignoring* matchers

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 331130.
steveire added a comment.
Herald added a project: clang-tools-extra.

Update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98296

Files:
  clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
  clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.h
  clang-tools-extra/clang-tidy/readability/DeletedDefaultCheck.cpp
  clang-tools-extra/clang-tidy/readability/DeletedDefaultCheck.h
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  
clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp
  
clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h
  clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
  clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.h
  clang-tools-extra/clang-tidy/readability/NamedParameterCheck.cpp
  clang-tools-extra/clang-tidy/readability/NamedParameterCheck.h
  clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.h
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.h
  clang-tools-extra/clang-tidy/readability/SimplifySubscriptExprCheck.cpp
  clang-tools-extra/clang-tidy/readability/SimplifySubscriptExprCheck.h
  
clang-tools-extra/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
  clang-tools-extra/clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
  clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
  clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h
  clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
  clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.h

Index: clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
===
--- clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
+++ clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
@@ -28,6 +28,9 @@
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
   void storeOptions(ClangTidyOptions::OptionMap ) override;
+  llvm::Optional getCheckTraversalKind() const override {
+return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   template 
Index: clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
@@ -196,12 +196,11 @@
   // Sadly, we can't check whether the literal has suffix or not.
   // E.g. i32 suffix still results in 'BuiltinType::Kind::Int'.
   // And such an info is not stored in the *Literal itself.
-  Finder->addMatcher(traverse(TK_AsIs,
+  Finder->addMatcher(
   stmt(eachOf(integerLiteral().bind(IntegerLiteralCheck::Name),
   floatLiteral().bind(FloatingLiteralCheck::Name)),
unless(anyOf(hasParent(userDefinedLiteral()),
-hasAncestor(isImplicit()),
-hasAncestor(substNonTypeTemplateParmExpr()),
+hasAncestor(substNonTypeTemplateParmExpr(),
   this);
 }
 
Index: clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h
===
--- clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h
+++ clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h
@@ -26,6 +26,9 @@
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
   void storeOptions(ClangTidyOptions::OptionMap ) override;
+  llvm::Optional getCheckTraversalKind() const override {
+return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   const bool PreferResetCall;
Index: clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
@@ -39,17 +39,14 @@
   Finder->addMatcher(
   cxxDeleteExpr(
   unless(isInTemplateInstantiation()),
-  has(expr(ignoringParenImpCasts(
-  cxxMemberCallExpr(
-  callee(
-  memberExpr(hasObjectExpression(allOf(
- unless(isTypeDependent()),
-  

[PATCH] D98712: [Utils] Support lit-like substitutions in update_cc_test_checks

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.



Comment at: llvm/utils/update_cc_test_checks.py:228
+  '%t' : tempfile.NamedTemporaryFile().name,
+  '%S' : os.getcwd(),
+}

Shouldn't this be the directory containing the test? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98712

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Thanks. I'm going to wait for some of the rocm people to pass judgement too as 
this code path is shared with hip / opencl etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D98741: [analyzer] Introduce common bug category "Unused code".

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist:11413
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

Charusso wrote:
> "Initialization was never used"? (Swift style: 
> https://swift.godbolt.org/z/c17EMb)
That's a scan-build bugtype so it follows a different tradition. The actual 
warning is a few lines above and it says "Value stored to 'foo' during its 
initialization is never read".

Additionally, unlike clang or swift (which [[ 
https://github.com/apple/swift/blob/main/docs/Diagnostics.md | inherits the 
tradition from clang ]]), static analyzer warnings are traditionally expected 
to be complete sentences. So we typically won't be able to synchronize wording.

But in this case it might be possible given that the warnings already have a 
similar structure. I'll think about it ^.^


Repository:
  rC Clang

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

https://reviews.llvm.org/D98741

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Johannes Doerfert via Phabricator via cfe-commits]

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

LG, it unbreaks the build and is not totally bananas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746

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

[PATCH] D98747: Thread safety analysis: Don't warn about managed locks on join points

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
aaronpuchert requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We already did so for scoped locks acquired in the constructor, this
change extends the treatment to deferred locks and scoped unlocking, so
locks acquired outside of the constructor. Obviously this makes things
more consistent.

Originally I thought this was a bad idea, because obviously it
introduces false negatives when it comes to double locking, but these
are typically easily found in tests, and the primary goal of the Thread
safety analysis is not to find double locks but race conditions.
Since the scoped lock will release the mutex anyway when the scope ends,
the inconsistent state is just temporary and probably fine.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98747

Files:
  clang/lib/Analysis/ThreadSafety.cpp
  clang/test/SemaCXX/warn-thread-safety-analysis.cpp


Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2595,6 +2595,7 @@
   if (c) {// test join point -- held/not held during release
 rlock.Release();
   }
+  // No warning on join point because the lock will be released by the scope 
object anyway.
 }
 
 void Foo::test3() {
@@ -2615,7 +2616,7 @@
   if (c) {
 rlock.Release();
   }
-  // no warning on join point for managed lock.
+  // No warning on join point because the lock will be released by the scope 
object anyway.
   rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not 
held}}
 }
 
@@ -2659,6 +2660,7 @@
 
 Mutex mu;
 int x GUARDED_BY(mu);
+bool b;
 
 void print(int);
 
@@ -2740,6 +2742,23 @@
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already 
held}}
 }
 
+void lockJoin() {
+  RelockableMutexLock scope(, DeferTraits{});
+  if (b)
+scope.Lock();
+  // No warning on join point because the lock will be released by the scope 
object anyway.
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 
'mu' exclusively}}
+}
+
+void unlockJoin() {
+  RelockableMutexLock scope(, DeferTraits{});
+  scope.Lock();
+  if (b)
+scope.Unlock();
+  // No warning on join point because the lock will be released by the scope 
object anyway.
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 
'mu' exclusively}}
+}
+
 void directUnlock() {
   RelockableExclusiveMutexLock scope();
   mu.Unlock();
@@ -2871,10 +2890,9 @@
 
 void join() EXCLUSIVE_LOCKS_REQUIRED(mu) {
   MutexUnlock scope();
-  if (c) {
-scope.Lock(); // expected-note{{mutex acquired here}}
-  }
-  // expected-warning@+1{{mutex 'mu' is not held on every path through here}}
+  if (c)
+scope.Lock();
+  // No warning on join point because the lock will be released by the scope 
object anyway.
   scope.Lock();
 }
 
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -983,7 +983,7 @@
 } else {
   FSet.removeLock(FactMan, !Cp);
   FSet.addLock(FactMan,
-   std::make_unique(Cp, kind, loc));
+   std::make_unique(Cp, kind, loc, true));
 }
   }
 
@@ -993,7 +993,7 @@
 if (FSet.findLock(FactMan, Cp)) {
   FSet.removeLock(FactMan, Cp);
   FSet.addLock(FactMan, std::make_unique(
-!Cp, LK_Exclusive, loc));
+!Cp, LK_Exclusive, loc, true));
 } else if (Handler) {
   SourceLocation PrevLoc;
   if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))


Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2595,6 +2595,7 @@
   if (c) {// test join point -- held/not held during release
 rlock.Release();
   }
+  // No warning on join point because the lock will be released by the scope object anyway.
 }
 
 void Foo::test3() {
@@ -2615,7 +2616,7 @@
   if (c) {
 rlock.Release();
   }
-  // no warning on join point for managed lock.
+  // No warning on join point because the lock will be released by the scope object anyway.
   rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
 }
 
@@ -2659,6 +2660,7 @@
 
 Mutex mu;
 int x GUARDED_BY(mu);
+bool b;
 
 void print(int);
 
@@ -2740,6 +2742,23 @@
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
 }
 
+void lockJoin() {
+  RelockableMutexLock scope(, DeferTraits{});
+  if (b)
+scope.Lock();
+  // No warning on join point because the lock will be 

[PATCH] D97121: [clang-tidy] Add a Standalone diagnostics mode to clang-tidy

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp:285
+#include "path/to/header2.h"
+#include "path/to/header.h"
+

njames93 wrote:
> steveire wrote:
> > I still find it really confusing that the "single inserter" mode results in 
> > multiple of the same header being added.  Perhaps the names should be along 
> > the lines of "duplicating" and "deduplicating" instead of "single" and 
> > "multi"?
> The name of the test is `InsertMultipleIncludesNoDeduplicate`, Is that not 
> sufficient?
Yes, the comment you're responding to here is quite old and predates the 
renaming.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97121

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

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield created this revision.
JonChesterfield added reviewers: yaxunl, t-tye, kzhuravl, ronlieb, b-sumner.
Herald added subscribers: kerbowa, pengfei, tpr, dstuttard, nhaehnle, jvesely.
JonChesterfield requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1, wdng.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

[clang][amdgpu] Use implicit code object default

At present, clang always passes amdhsa-code-object-version on to -cc1. That is
great for certainty over what object version is being used when debugging.

Unfortunately, the command line argument is in AMDGPUBaseInfo.cpp in the amdgpu
target. If clang is used with an llvm compiled with DLLVM_TARGETS_TO_BUILD
that excludes amdgpu, this will be diagnosed (as discovered via D98658 
):

- Unknown command line argument '--amdhsa-code-object-version=3'

This means that clang, built only for X86, can be used to compile the nvptx
devicertl for openmp but not the amdgpu one. That would shortly spawn fragile
logic in the devicertl cmake to try to guess whether the clang used will work.

This change omits the amdhsa-code-object-version parameter when it matches the
default that AMDGPUBaseInfo.cpp specifies, with a comment to indicate why. As
this is the only part of clang's codegen for amdgpu that depends on the target
in the back end it suffices to build the openmp runtime on most (all?) systems.

It is a non-functional change, though observable in the updated tests and when
compiling with -###. It may cause minor disruption to the amd-stg-open branch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98746

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/amdgpu-features-as.s
  clang/test/Driver/amdgpu-features.c


Index: clang/test/Driver/amdgpu-features.c
===
--- clang/test/Driver/amdgpu-features.c
+++ clang/test/Driver/amdgpu-features.c
@@ -1,6 +1,5 @@
 // RUN: %clang -### -target amdgcn-amd-amdhsa -mcpu=gfx700 -mcode-object-v3 %s 
2>&1 | FileCheck --check-prefix=CODE-OBJECT-V3 %s
 // CODE-OBJECT-V3: warning: argument '-mcode-object-v3' is deprecated, use 
'-mcode-object-version=3' instead [-Wdeprecated]
-// CODE-OBJECT-V3: "-mllvm" "--amdhsa-code-object-version=3"
 
 // RUN: %clang -### -target amdgcn-amd-amdhsa amdgcn -mcpu=gfx700 
-mno-code-object-v3 %s 2>&1 | FileCheck --check-prefix=NO-CODE-OBJECT-V3 %s
 // NO-CODE-OBJECT-V3: warning: argument '-mno-code-object-v3' is deprecated, 
use '-mcode-object-version=2' instead [-Wdeprecated]
@@ -8,7 +7,6 @@
 
 // RUN: %clang -### -target amdgcn-amd-amdhsa -mcpu=gfx700 -mcode-object-v3 
-mno-code-object-v3 -mcode-object-v3 %s 2>&1 | FileCheck 
--check-prefix=MUL-CODE-OBJECT-V3 %s
 // MUL-CODE-OBJECT-V3: warning: argument '-mcode-object-v3' is deprecated, use 
'-mcode-object-version=3' instead [-Wdeprecated]
-// MUL-CODE-OBJECT-V3: "-mllvm" "--amdhsa-code-object-version=3"
 
 // RUN: %clang -### -target amdgcn-amdhsa -mcpu=gfx900:xnack+ %s 2>&1 | 
FileCheck --check-prefix=XNACK %s
 // XNACK: "-target-feature" "+xnack"
Index: clang/test/Driver/amdgpu-features-as.s
===
--- clang/test/Driver/amdgpu-features-as.s
+++ clang/test/Driver/amdgpu-features-as.s
@@ -1,6 +1,5 @@
 // RUN: %clang -### -target amdgcn-amd-amdhsa -mcpu=gfx900 -mcode-object-v3 %s 
2>&1 | FileCheck --check-prefix=CODE-OBJECT-V3 %s
 // CODE-OBJECT-V3: warning: argument '-mcode-object-v3' is deprecated, use 
'-mcode-object-version=3' instead [-Wdeprecated]
-// CODE-OBJECT-V3: "-mllvm" "--amdhsa-code-object-version=3"
 
 // RUN: %clang -### -target amdgcn-amd-amdhsa amdgcn -mcpu=gfx900 
-mno-code-object-v3 %s 2>&1 | FileCheck --check-prefix=NO-CODE-OBJECT-V3 %s
 // NO-CODE-OBJECT-V3: warning: argument '-mno-code-object-v3' is deprecated, 
use '-mcode-object-version=2' instead [-Wdeprecated]
@@ -8,4 +7,3 @@
 
 // RUN: %clang -### -target amdgcn-amd-amdhsa -mcpu=gfx900 -mcode-object-v3 
-mno-code-object-v3 -mcode-object-v3 %s 2>&1 | FileCheck 
--check-prefix=MUL-CODE-OBJECT-V3 %s
 // MUL-CODE-OBJECT-V3: warning: argument '-mcode-object-v3' is deprecated, use 
'-mcode-object-version=3' instead [-Wdeprecated]
-// MUL-CODE-OBJECT-V3: "-mllvm" "--amdhsa-code-object-version=3"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1112,10 +1112,15 @@
  const ArgList ,
  ArgStringList ) {
   unsigned CodeObjVer = getOrCheckAMDGPUCodeObjectVersion(D, Args);
-  CmdArgs.insert(CmdArgs.begin() + 1,
- Args.MakeArgString(Twine("--amdhsa-code-object-version=") +
-Twine(CodeObjVer)));
-  CmdArgs.insert(CmdArgs.begin() + 1, 

[PATCH] D98745: [clang] Add fixit for Wreorder-ctor

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: rsmith, sammccall, aaron.ballman.
Herald added a subscriber: mgrang.
njames93 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Create fix-it hints to fix the order of constructors.
To make this a lot simpler, I've grouped all the warnings for each out of order 
initializer into 1.
This is necessary as fixing one initializer would often interfere with other 
initializers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98745

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/FixIt/fixit-cxx-init-order.cpp
  clang/test/SemaCXX/constructor-initializer.cpp
  clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp

Index: clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
===
--- clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
+++ clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
@@ -7,10 +7,10 @@
 class complex : public BB, BB1 { 
 public: 
   complex()
-: s2(1), // expected-warning {{field 's2' will be initialized after field 's1'}}
+: s2(1), // expected-warning {{some initializers aren't given in the correct order}} expected-note {{field 's2' will be initialized after field 's1'}}
   s1(1),
-  s3(3), // expected-warning {{field 's3' will be initialized after base 'BB1'}} 
-  BB1(), // expected-warning {{base class 'BB1' will be initialized after base 'BB'}}
+  s3(3), // expected-note {{field 's3' will be initialized after base 'BB1'}} 
+  BB1(), // expected-note {{base class 'BB1' will be initialized after base 'BB'}}
   BB()
   {}
   int s1;
Index: clang/test/SemaCXX/constructor-initializer.cpp
===
--- clang/test/SemaCXX/constructor-initializer.cpp
+++ clang/test/SemaCXX/constructor-initializer.cpp
@@ -91,8 +91,9 @@
 
 struct Current : Derived {
   int Derived;
-  Current() : Derived(1), ::Derived(), // expected-warning {{field 'Derived' will be initialized after base '::Derived'}} \
-   // expected-warning {{base class '::Derived' will be initialized after base 'Derived::V'}}
+  Current() : Derived(1), ::Derived(), // expected-warning {{some initializers aren't given in the correct order}} \
+   // expected-note {{field 'Derived' will be initialized after base '::Derived'}} \
+   // expected-note {{base class '::Derived' will be initialized after base 'Derived::V'}}
   ::Derived::Base(), // expected-error {{type '::Derived::Base' is not a direct or virtual base of 'Current'}}
Derived::Base1(), // expected-error {{type 'Derived::Base1' is not a direct or virtual base of 'Current'}}
Derived::V(),
Index: clang/test/FixIt/fixit-cxx-init-order.cpp
===
--- /dev/null
+++ clang/test/FixIt/fixit-cxx-init-order.cpp
@@ -0,0 +1,22 @@
+// Due to the fix having multiple edits we can't use
+// '-fdiagnostics-parseable-fixits' to determine if fixes are correct. However,
+// running fixit recompile with 'Werror' should fail if the fixes are invalid.
+
+// RUN: %clang_cc1 %s -Werror=reorder-ctor -fixit-recompile -fixit-to-temporary
+// RUN: %clang_cc1 %s -Wreorder-ctor -verify -verify-ignore-unexpected=note
+
+struct Foo {
+  int A, B, C;
+
+  Foo() : A(1), B(3), C(2) {}
+  Foo(int) : A(1), C(2), B(3) {}  // expected-warning {{field 'C' will be initialized after field 'B'}}
+  Foo(unsigned) : C(2), B(3), A(1) {} // expected-warning {{some initializers aren't given in the correct order}}
+};
+
+struct Bar : Foo {
+  int D, E, F;
+
+  Bar() : Foo(), D(1), E(2), F(3) {}
+  Bar(int) : D(1), E(2), F(3), Foo(4) {}  // expected-warning {{field 'F' will be initialized after base 'Foo'}}
+  Bar(unsigned) : F(3), E(2), D(1), Foo(4) {} // expected-warning {{some initializers aren't given in the correct order}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -5239,6 +5239,20 @@
   return Member->getAnyMember()->getCanonicalDecl();
 }
 
+static void AddInitializerToDiag(const Sema::SemaDiagnosticBuilder ,
+ const CXXCtorInitializer *Previous,
+ const CXXCtorInitializer *Current) {
+  if (Previous->isAnyMemberInitializer())
+Diag << 0 << Previous->getAnyMember()->getDeclName();
+  else
+Diag << 1 << Previous->getTypeSourceInfo()->getType();
+
+  if (Current->isAnyMemberInitializer())
+Diag << 0 << Current->getAnyMember()->getDeclName();
+  else
+Diag << 1 << Current->getTypeSourceInfo()->getType();
+}
+
 

[PATCH] D98191: [flang][driver] Add support for `-fdebug-dump-symbols-sources`

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D98191#2611694 , @tskeith wrote:

> `-fget-symbols-sources` is not a debug option, it's intended for integrating 
> with IDEs like vscode. So I think the original name is better. Unlike the 
> "dump" options it actually is an action and not something that is intended to 
> produce debug output on the way to doing something else.

A drive-through comment.
Do we plan to use a compiler option to integrate with IDEs?  I was thinking the 
integration will be through another interface and not through the compiler 
driver and that the fget-symbols-sources is a temporary option to test the 
functionality. clangd which supports IDEs is a separate tool.
https://github.com/llvm/llvm-project/tree/main/clang-tools-extra/clangd


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98191

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

[PATCH] D98429: [clang-format] Add new option to clang-format: SpaceBeforeForLoopSemiColon

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.

Could you point me to a style guide that formats the code in this way please?




Comment at: clang/include/clang/Format/Format.h:2838
 
+  /// If ``false``, spaces will be removed before semi-colons in for loops.
+  /// \code

A better description would be nice, here only the false case is described. I'd 
rather see something that describes the option is in general and adds that true 
will add/keep a space.



Comment at: clang/lib/Format/TokenAnnotator.cpp:4004
 return false;
+  if (Right.is(TT_ForLoopSemiColon))
+return false;

Is this change really necessary? Is there a test for it?
I guess that a semicolon should return false because otherwise it could be put 
after the line break, but you certainly need a test for that. 



Comment at: clang/unittests/Format/FormatTest.cpp:12712
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+  verifyFormat("for (int i = 0 ; auto a : b) {\n}", Space);
+}

HazardyKnusperkeks wrote:
> hjelmn wrote:
> > HazardyKnusperkeks wrote:
> > > Okay that was unexpected for me, I thought the space would only apply to 
> > > the old `for`s.
> > > 
> > > In that case please add `while` and maybe `if` with initializer. What 
> > > should be discussed is if `for` and the other control statements with 
> > > initializer should behave differently with regard to their initializer 
> > > semicolon.
> > hmm, I think then I could rename this one to: SpaceBeforeCForLoopSemiColon 
> > which would then only add spaces in for(init;cond;increment)
> > then maybe SpaceAfterControlStatementInitializer or 
> > SpaceBeforeControlStatementSemiColon that controls the behavior for control 
> > statements with initializers.
> > 
> > How does that sound?
> Apart from the names good. For the names I don't have anything really better 
> right now.
> 
> But this is currently just my opinion, we should ask @MyDeveloperDay and 
> @curdeius.
I would prefer to avoid multiplying the different options and regroup all of 
the control statements under a single one.
`SpaceBeforeControlStatementSemicolon` sounds acceptable with one nit, I'd use 
"Semicolon" (as a single word, with lowercase 'c') to match the usage in LLVM 
(e.g. in clang-tidy).

WDYT about a single option for all statements?
Another way is to add a separate option for each statement type. Or, use a 
(bitmask like) enum with a single option. The latter can be done in a backward 
compatible way in the future BTW.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98429

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

[PATCH] D97916: [Driver][RISCV] Support parsing multi-lib config from GCC.

[Luís Marques via Phabricator via cfe-commits]

luismarques added a comment.

This still doesn't report that the multilib configuration came from GCC when it 
succeeds, does it? I suppose that's not a deal-breaker, but it would be nice to 
have. Would it be difficult to implement?

Regarding the Windows test issue, aren't there other test cases in similar 
situations that managed to sort that out?




Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1659
+  for (StringRef Line : Lines) {
+if (Line.trim().empty())
+  continue;

I was actually thinking something along the lines of `Line = Line.trim();`, so 
that the rest of the parsing code would also benefit from having any extraneous 
whitespace removed and thus be more robust. Or does this mutate the argument?



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1742
+  if (RC != 0) {
+MultilibErrorMessages = "Read GCC multilib configureation failed due to "
+"non-zero return code";

configureation -> configuration. Also, the user won't know what the "non-zero 
return code" refers to. How about something like "Failed to execute  in 
an attempt to obtain the multilib configuration from GCC"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97916

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

[PATCH] D98741: [analyzer] Introduce common bug category "Unused code".

[Csaba Dabis via Phabricator via cfe-commits]

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

Great idea!




Comment at: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist:11413
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

"Initialization was never used"? (Swift style: 
https://swift.godbolt.org/z/c17EMb)


Repository:
  rC Clang

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

https://reviews.llvm.org/D98741

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

[PATCH] D93164: [AST] Add generator for source location introspection

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

I am seeing a spew of errors after 19740652c4c4329e2b9e77f96e5e31c360b4e8bb 
 (what 
appears to be the latest version of this patch):

  $ cmake \
  -G Ninja \
  -DCMAKE_BUILD_TYPE=Release \
  -DCMAKE_C_COMPILER=$(command -v clang) \
  -DCMAKE_CXX_COMPILER=$(command -v clang++) \
  -DLLVM_CCACHE_BUILD=ON \
  -DLLVM_ENABLE_PROJECTS=clang \
  ../llvm
  ...
  
  $ ninja run-ast-api-dump-tool
  ...
  In file included from 
/home/nathan/src/llvm-project/build/tools/clang/lib/Tooling/ASTTU.cpp:2:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/AST.h:17:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/ASTContext.h:19:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/CanonicalType.h:17:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/Type.h:20:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/DependenceFlags.h:11:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/Basic/BitmaskEnum.h:18:
  In file included from 
/home/nathan/src/llvm-project/llvm/include/llvm/ADT/BitmaskEnum.h:16:
  In file included from 
/home/nathan/src/llvm-project/llvm/include/llvm/Support/MathExtras.h:21:
  /usr/include/c++/10.2.0/cstdint:52:11: error: no member named 'int_fast8_t' 
in the global namespace
using ::int_fast8_t;
  ~~^
  /usr/include/c++/10.2.0/cstdint:53:11: error: no member named 'int_fast16_t' 
in the global namespace; did you mean '__int_least16_t'?
using ::int_fast16_t;
  ~~^
  /usr/include/bits/types.h:54:19: note: '__int_least16_t' declared here
  typedef __int16_t __int_least16_t;
^
  In file included from 
/home/nathan/src/llvm-project/build/tools/clang/lib/Tooling/ASTTU.cpp:2:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/AST.h:17:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/ASTContext.h:19:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/CanonicalType.h:17:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/Type.h:20:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/DependenceFlags.h:11:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/Basic/BitmaskEnum.h:18:
  In file included from 
/home/nathan/src/llvm-project/llvm/include/llvm/ADT/BitmaskEnum.h:16:
  In file included from 
/home/nathan/src/llvm-project/llvm/include/llvm/Support/MathExtras.h:21:
  /usr/include/c++/10.2.0/cstdint:54:11: error: no member named 'int_fast32_t' 
in the global namespace; did you mean '__int_least32_t'?
using ::int_fast32_t;
  ~~^
  /usr/include/bits/types.h:56:19: note: '__int_least32_t' declared here
  typedef __int32_t __int_least32_t;
^
  In file included from 
/home/nathan/src/llvm-project/build/tools/clang/lib/Tooling/ASTTU.cpp:2:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/AST.h:17:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/ASTContext.h:19:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/CanonicalType.h:17:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/Type.h:20:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/AST/DependenceFlags.h:11:
  In file included from 
/home/nathan/src/llvm-project/llvm/../clang/include/clang/Basic/BitmaskEnum.h:18:
  In file included from 
/home/nathan/src/llvm-project/llvm/include/llvm/ADT/BitmaskEnum.h:16:
  In file included from 
/home/nathan/src/llvm-project/llvm/include/llvm/Support/MathExtras.h:21:
  /usr/include/c++/10.2.0/cstdint:55:11: error: no member named 'int_fast64_t' 
in the global namespace; did you mean '__int_least64_t'?
using ::int_fast64_t;
  ~~^
  /usr/include/bits/types.h:58:19: note: '__int_least64_t' declared here
  typedef __int64_t __int_least64_t;
^
  ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93164

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

[PATCH] D98741: [analyzer] Introduce common bug category "Unused code".

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: vsavchenko, xazax.hun, Szelethus, martong, 
baloghadamsoftware, Charusso, steakhal, balazske, ASDenysPetrov.
Herald added subscribers: dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, 
rnkovacs, szepet.
NoQ requested review of this revision.

This is a cosmetic change. "Dead store" will now be displayed as "Unused code" 
which is a nice broad category that could incorporate more than one checker. It 
also doesn't mention dead people which despite being a common source of inside 
jokes in the static analyzer community doesn't need to be translated onto 
innocent users.

There's one more alpha checker that fits into the category, namely 
UnreachableCode checker which flags code that wasn't covered by symbolic 
execution. I don't immediately plan to actually make this checker useful as it 
has to be quite an undertaking.

A broader plan that i have here is that some clang-tidy checks (eg., 
bugprone-redundant-branch-condition or misc-redundant-expression) can be put 
into that category through D95403 .


Repository:
  rC Clang

https://reviews.llvm.org/D98741

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
  clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
  clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist

Index: clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -2169,7 +2169,7 @@
 

descriptionValue stored to foo during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -5654,7 +5654,7 @@
 

descriptionValue stored to x is never read
-   categoryDead store
+   categoryUnused code
typeDead increment
check_namedeadcode.DeadStores

Index: clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
@@ -382,7 +382,7 @@
 

descriptionValue stored to x during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -450,7 +450,7 @@
 

descriptionValue stored to obj1 during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -518,7 +518,7 @@
 

descriptionValue stored to obj4 during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -586,7 +586,7 @@
 

descriptionValue stored to obj5 during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -654,7 +654,7 @@
 

descriptionValue stored to obj6 during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -1064,7 +1064,7 @@
 

descriptionValue stored to cf1 during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -1132,7 +1132,7 @@
 

descriptionValue stored to cf2 during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -1200,7 +1200,7 @@
 

descriptionValue stored to cf3 during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

@@ -1268,7 +1268,7 @@
 

descriptionValue stored to cf4 during its initialization is never read
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

Index: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
===
--- clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -2368,7 +2368,7 @@
 

descriptionValue stored to x is never read
-   categoryDead store
+   categoryUnused code
typeDead increment
check_namedeadcode.DeadStores

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

@NoQ, could you upstream it and move this idea forward please?


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

https://reviews.llvm.org/D69726

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

[PATCH] D98502: [clang][Checkers] Extend PthreadLockChecker state dump (NFC).

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

That's what i like to see!

You can test this via `clang_analyzer_printState()`.




Comment at: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:343
+  if (!DRV.isEmpty()) {
+Out << Sep << "Mutex destroys with unknown result:" << NL;
+for (auto I : DRV) {

I think this should be passive. The mutex doesn't actively destroy anybody.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98502

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

[PATCH] D98738: [clang-tidy] performance-* checks: Match AllowedTypes against qualified type names when they contain "::".

[Felix Berger via Phabricator via cfe-commits]

flx created this revision.
flx added reviewers: ymandel, aaron.ballman.
Herald added a subscriber: xazax.hun.
flx requested review of this revision.
Herald added a project: clang-tools-extra.

This allows users to be more precise and exclude a type in a specific namespace
from triggering the check instead of excluding all types with the same
unqualified name.

This change should not interfere with correctly configured clang-tidy setups
since an AllowedType with "::" would never match.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98738

Files:
  clang-tools-extra/clang-tidy/utils/Matchers.h
  clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-value-param.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy-allowed-types.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy-allowed-types.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy-allowed-types.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy-allowed-types.cpp
@@ -1,5 +1,5 @@
 // RUN: %check_clang_tidy %s performance-for-range-copy %t -- \
-// RUN: -config="{CheckOptions: [{key: 
performance-for-range-copy.AllowedTypes, value: 
'[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$'}]}" \
+// RUN: -config="{CheckOptions: [{key: 
performance-for-range-copy.AllowedTypes, value: 
'[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$;fully::Qualified'}]}" \
 // RUN: -- -fno-delayed-template-parsing
 
 template 
@@ -63,6 +63,14 @@
 
 typedef SomeComplexTemplate NotTooComplexRef;
 
+namespace fully {
+
+struct Qualified {
+  ~Qualified();
+};
+
+} // namespace fully
+
 void negativeSmartPointer() {
   for (auto P : View>()) {
 auto P2 = P;
@@ -124,3 +132,13 @@
 auto R2 = R;
   }
 }
+
+void negativeFullyQualified() {
+  for (auto Q : View>()) {
+auto Q2 = Q;
+  }
+  using fully::Qualified;
+  for (auto Q : View>()) {
+auto Q2 = Q;
+  }
+}
Index: 
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-value-param.rst
===
--- 
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-value-param.rst
+++ 
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-value-param.rst
@@ -66,4 +66,7 @@
 
A semicolon-separated list of names of types allowed to be passed by value.
Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches every type
-   with suffix `Ref`, `ref`, `Reference` and `reference`. The default is empty.
+   with suffix `Ref`, `ref`, `Reference` and `reference`. The default is
+   empty. If a name in the list contains the sequence `::` it is matched 
against
+   the qualified typename (i.e. `namespace::Type`, otherwise it is matched
+   against only the type name (i.e. `Type`).
Index: 
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
===
--- 
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
+++ 
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
@@ -43,5 +43,7 @@
 
A semicolon-separated list of names of types allowed to be initialized by
copying. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches
-   every type with suffix `Ref`, `ref`, `Reference` and `reference`. The
-   default is empty.
+   every type with suffix `Ref`, `ref`, `Reference` and `reference`. The 
default
+   is empty. If a name in the list contains the sequence `::` it is matched
+   against the qualified typename (i.e. `namespace::Type`, otherwise it is
+   matched against only the type name (i.e. `Type`).
Index: clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst
+++ clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst
@@ -31,4 +31,6 @@
A semicolon-separated list of names of types allowed to be copied in each
iteration. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches
every type with suffix `Ref`, `ref`, `Reference` and `reference`. The 
default
-   is empty.
+   is empty. If a name in the list contains the sequence `::` it is matched
+   against the qualified typename (i.e. `namespace::Type`, otherwise it is
+   matched against only the type name (i.e. `Type`).
Index: clang-tools-extra/clang-tidy/utils/Matchers.h
===
--- clang-tools-extra/clang-tidy/utils/Matchers.h
+++ clang-tools-extra/clang-tidy/utils/Matchers.h
@@ -52,8 +52,11 

[PATCH] D98726: [analyzer] Remove unnecessary TODO

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D98726#2629722 , @steakhal wrote:

> For example this code does not trigger any warning:
> https://godbolt.org/z/aM4xe7

FTFY: https://godbolt.org/z/vT5c7s

This test isn't very interesting though because the warning doesn't have 
anything to do with the smart pointer. Any use of the pointer after `delete` 
deserves a warning regardless of whether it's coming from the smart pointer or 
not. In other words, in this test case MallocChecker doesn't start tracking the 
pointer at `release()` but it only starts tracking the pointer later, at 
`delete`.

A more interesting test would be to diagnose a leak if the pointer isn't 
deleted. Which we don't do: https://godbolt.org/z/1EczEW So, yeah, i think this 
TODO is still relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98726

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

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/Decl.h:82
+enum ReservedIdentifierReason {
+  StartsWithUnderscoreAtGlobalScope,
+  StartsWithDoubleUnderscore,

Because this is not a scoped enum, should these enumerators get an `RIR_` 
prefix added to them?



Comment at: clang/include/clang/AST/Decl.h:368-369
+  /// given language.
+  bool isReserved(const LangOptions &,
+  ReservedIdentifierReason *Reason = nullptr) const;
+

Any reason not to return the enumeration as the result? We could add 
`NotReserved = 0` to the enumeration so that calls to `D->isReserved()` will do 
the right thing.

(Also, might as well name the `LangOptions` parameter here.)



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:790
+
+def ReservedIdAsSymbol : DiagGroup<"reserved-extern-identifier">;
+def ReservedIdentifier : DiagGroup<"reserved-identifier",

Were you planning to use this new diagnostic group?



Comment at: clang/lib/AST/Decl.cpp:1084
+  const IdentifierInfo *II = nullptr;
+  if (auto *FD = dyn_cast(this))
+II = FD->getLiteralIdentifier();

Might as well address the lint warning.



Comment at: clang/lib/AST/Decl.cpp:1104-1105
+// Walk up the lexical parents to determine if we're at TU level or not.
+const DeclContext * DC = getLexicalDeclContext();
+while(DC->isTransparentContext())
+  DC = DC->getLexicalParent();




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

https://reviews.llvm.org/D93095

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

[PATCH] D97850: Fix PCM read from ModuleCache for ext4 filesystem

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D97850#2629464 , @ilyakuteev wrote:

> If a fix will be in ModuleManager and only for ModuleCache the problem with 
> symlinks and path will not affect it as ModuleCache is managed by clang and 
> we can rely on it.
> I agree that using `FileMgr.getBypassFile` is not the best way to solve this 
> problem, we need to replace `FileMgr.getFileRef` with some other method but I 
> did not found such method in FileManager. Maybe we need to add one or not use 
> FileManager as was mentioned. Not sure which way is better and safer.

FileManager plays two roles (unless I'm missing a third?):

- Establish an identity for multiple paths that should be treated as "the same" 
(address of FileEntry).
- Cache stat information (the content of FileEntry).

This patch is predicated on it being safe to skip the former (makes sense to 
me, although I think clang is a bit inconsistent about using relative paths for 
the module cache, so there might be some work to do). We already skip / avoid 
the latter (ModuleManager is the only caller of 
FileManager::getNoncachedStatValue). Seems like skipping the FileManager will 
simplify both the ModuleManager and the FileManager.

Might be worth an RFC on cfe-dev?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97850

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

[PATCH] D93594: [X86] Pass to transform amx intrinsics to scalar operation.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

@yubing In this case I would recommend building `sqlite3.c` from test-suite 
under `perf stat` and look at the `instructions` metric. For me the command 
looks like this:

  perf stat CLANG_BINARY   -w -Werror=date-time -DSTDC_HEADERS=1 
-DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 
-DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 
-DHAVE_UNISTD_H=1 -DSQLITE_OMIT_LOAD_EXTENSION=1 -DSQLITE_THREADSAFE=0 -I. -MD 
-MT MultiSource/Applications/sqlite3/CMakeFiles/sqlite3.dir/sqlite3.c.o -MF 
MultiSource/Applications/sqlite3/CMakeFiles/sqlite3.dir/sqlite3.c.o.d -o 
MultiSource/Applications/sqlite3/CMakeFiles/sqlite3.dir/sqlite3.c.o   -c 
../MultiSource/Applications/sqlite3/sqlite3.c

You can generally get a build command using `ninja -v sqlite3` in test-suite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93594

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

[PATCH] D98622: [-Wcalled-once-parameter] Let escapes overwrite MaybeCalled states

[Artem Dergachev via Phabricator via cfe-commits]

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

LGTM! Do any of the high-level comments at the beginning of the file need to be 
updated to reflect this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98622

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

[PATCH] D98429: [clang-format] Add new option to clang-format: SpaceBeforeForLoopSemiColon

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a subscriber: curdeius.
HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:168
 
+- Option ``SpaceBeforeForLoopSemiColon`` has been added to control whether
+  spaces will be added before the semi-colons in for loops.

hjelmn wrote:
> HazardyKnusperkeks wrote:
> > No need for change, but in the future I would prefer to add changes to the 
> > end of the list. :)
> Opps. Should have asked the convention before putting it at the top. I will 
> go ahead and move it to be consistent with LLVM practice.
Don't know about LLVM practice, I'm just about 4 month here. Just my preference 
(and how I've done it until now).



Comment at: clang/unittests/Format/FormatTest.cpp:12712
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+  verifyFormat("for (int i = 0 ; auto a : b) {\n}", Space);
+}

hjelmn wrote:
> HazardyKnusperkeks wrote:
> > Okay that was unexpected for me, I thought the space would only apply to 
> > the old `for`s.
> > 
> > In that case please add `while` and maybe `if` with initializer. What 
> > should be discussed is if `for` and the other control statements with 
> > initializer should behave differently with regard to their initializer 
> > semicolon.
> hmm, I think then I could rename this one to: SpaceBeforeCForLoopSemiColon 
> which would then only add spaces in for(init;cond;increment)
> then maybe SpaceAfterControlStatementInitializer or 
> SpaceBeforeControlStatementSemiColon that controls the behavior for control 
> statements with initializers.
> 
> How does that sound?
Apart from the names good. For the names I don't have anything really better 
right now.

But this is currently just my opinion, we should ask @MyDeveloperDay and 
@curdeius.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98429

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Xun Li via Phabricator via cfe-commits]

lxfind added inline comments.



Comment at: clang/include/clang/AST/ExprCXX.h:4695
+/// afterwards on the stack.
 class CoroutineSuspendExpr : public Expr {
   friend class ASTStmtReader;

ChuanqiXu wrote:
> It looks strange for the change of `CoroutineSuspendExpr` at the first 
> glance. It is easy to understand the coroutine suspend expression is consists 
> of three parts: Ready, Suspend and resume. It is written in the language 
> documentation. And the new added AwaitSuspendCall is confusing.
I agree. But this seems to be the only way to break up Suspend at the point of 
await_suspend call so that we can insert instructions during CodeGen. Open to 
ideas though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:207
+
+  LLVM_DEBUG(QT.dump());
+

Actually this is one of those debug prints that should be removed and remained 
in here by accident.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs() || RType->getAs()) {
+// Unfortunately, the canonical type of a function pointer becomes the
+// same even if exactly one is "noexcept" and the other isn't, making us
+// give a false positive report irrespective of implicit conversions.
+LLVM_DEBUG(llvm::dbgs()

@aaron.ballman Getting ahead of the curve here. I understand this is ugly. 
Unfortunately, no matter how much I read the standard, I don't get anything of 
"canonical type" mentioned, it seems to me this concept is something inherent 
to the model of Clang.

Basically why this is here: imagine a `void (*)() noexcept` and a `void (*)()`. 
One's `noexcept`, the other isn't. Inside the AST, this is a `ParenType` of a 
`PointerType` to a `FunctionProtoType`. There exists a //one-way// implicit 
conversion from the `noexcept` to the non-noexcept ("noexceptness can be 
discarded", similarly to how "constness can be gained")
Unfortunately, because this is a one-way implicit conversion, it won't return 
on the code path earlier for implicit conversions.

Now because of this, the recursive descent in our code will reach the point 
when the two innermost `FunctionProtoType`s are in our hands. As a fallback 
case, we simply ask Clang "Hey, do //you// think these two are the same?". And 
for some weird reason, Clang will say "Yes."... While one of them is a 
`noexcept` function and the other one isn't.

I do not know the innards of the AST well enough to even have a glimpse of 
whether or not this is a bug. So that's the reason why I went ahead and 
implemented this side-stepping logic. Basically, as the comment says, it'lll 
**force** the information of "No matter what you do, do NOT consider these 
mixable!" back up the recursion chain, and handle it appropriately later.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp:250-260
+using NonThrowingFunction = void (*)() noexcept;
+
+struct NoexceptMaker {
+  NoexceptMaker(void (*ThrowingFunction)());
+  // Need to use a typedef here because
+  // "conversion function cannot convert to a function type".
+  // operator (void (*)() noexcept) () const;

Now here the warning is justified, however. All these disinfectant-stinking 
examples are to ensure that. In this case, in our hands, on the first level, we 
got the function pointer, and the class. And it **is** the implicit conversion 
modeller that will eventually ask "Can I give that noexcept function to that 
constructor taking a non-noexcept?" and in that case, on the level of function 
pointers, the answer is //yes//, so there will be a warning made.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:336-337
+
+void functionPrototypeLosesNoexcept(void (*NonThrowing)() noexcept, void 
(*Throwing)()) {}
+// NO-WARN: This call cannot be swapped, even if "getCanonicalType()" believes 
otherwise.

Without this side-stepping logic mentioned above, even **without this patch** 
(so back to the "Ye Olde Strict Type Equality" mode), this particular function 
would be warned about, which is a definite false positive.


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

https://reviews.llvm.org/D75041

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

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

In D98638#2628082 , @ChuanqiXu wrote:

> It looks like there are two things this patch wants to do:
>
> 1. Don't put the temporary generated by symmetric-transfer on the coroutine 
> frame.
> 2. Offer a mechanism to force some values (it is easy to extend Alloca to 
> Value) to put in the stack instead of the coroutine frame.
>
> I am a little confused about the first problem. Would it cause the program to 
> crash? (e.g., we access the fields of coroutine frame after the frame gets 
> destroyed). Or it just wastes some storage?
> And I want to ask about the change of the AST nodes and SemaCoroutine. Can we 
> know if a CoroutineSuspendExpr stands for a symmetric-transfer? If yes, it 
> seems we can only do changes in CodeGen part.

It will result in a crash, because we will be accessing memory that's already 
freed.  If you run:

  bin/clang -fcoroutines-ts -std=c++14 -stdlib=libc++ 
../clang/test/CodeGenCoroutines/coro-symmetric-transfer-01.cpp -o - -emit-llvm 
-S -Xclang -disable-llvm-passes

You can see that in the `final.suspend` basic block, there are IRs like this:

%call19 = call i8* 
@_ZN13detached_task12promise_type13final_awaiter13await_suspendENSt12experimental13coroutines_v116coroutine_handleIS0_EE(%"struct.detached_task::promise_type::final_awaiter"*
 nonnull dereferenceable(1) %ref.tm
  p10, i8* %22) #2
%coerce.dive20 = getelementptr inbounds 
%"struct.std::experimental::coroutines_v1::coroutine_handle.0", 
%"struct.std::experimental::coroutines_v1::coroutine_handle.0"* %coerce, i32 0, 
i32 0
store i8* %call19, i8** %coerce.dive20, align 8
%call21 = call i8* 
@_ZNKSt12experimental13coroutines_v116coroutine_handleIvE7addressEv(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"*
 nonnull dereferenceable(8) %coerce) #2
call void @llvm.coro.resume(i8* %call21)

The temporary variable %coerce will be put on the frame because it's used by 
the call to `address` function and LLVM thinks it may escape. But the call to 
await_suspend() (the first line) in reality could destroy the current coroutine 
frame. Hence after the call to await_suspend, it will be accessing the frame, 
leading to memory corruption.

> Then I agree to introduce new intrinsic to hint the middle end to put some 
> values on the stack. And the design of `@llvm.coro.forcestack.begin()` and 
> `@llvm.coro.forcestack.end()` is a little strange to me. It says they mark a 
> region where only data from the local stack can be accessed. But it looks 
> error-prone since it is hard for the front-end to decide whether all the 
> access of the region should be put on the stack. I think we could introduce 
> only one intrinisic `@llvm.coro.forcestack(Value* v)`, we can use the 
> argument to mark the value need to be put on the stack.

This is a good idea. Let me play with it. Thanks!

> And about the problem you mentioned in D96922 
> : "The lifetime of  %coro.gro" starts early 
> and %coro.gro" would be used after `coro.end` (Possibly the destructor?) 
> which would cause the program to access destroyed coroutine frame". It looks 
> like the mechanism could solve this problem by a call to 
> `@llvm.coro.forcestack(%coro.gro)`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98638

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

[PATCH] D98635: [libtooling][clang-tidy] Fix diagnostics not respecting and highlighting fed SourceRanges

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D98635#2629827 , @whisperity wrote:

> @aaron.ballman Could you please help me understand this pre-merge buildbot? 
> It says that Debian failed, with the following message:
>
>   [15/1038] Running lint check for sanitizer sources...
>   FAILED: projects/compiler-rt/lib/CMakeFiles/SanitizerLintCheck
>   cd /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/lib && env 
> LLVM_CHECKOUT=/mnt/disks/ssd0/agent/llvm-project/llvm SILENT=1 TMPDIR= 
> PYTHON_EXECUTABLE=/usr/bin/python3.8 
> COMPILER_RT=/mnt/disks/ssd0/agent/llvm-project/compiler-rt 
> /mnt/disks/ssd0/agent/llvm-project/compiler-rt/lib/sanitizer_common/scripts/check_lint.sh
>   The following differences between the ABI list and the implemented custom 
> wrappers have been found:
>
> Should I be worried because of this, or is this that the patch started 
> building when the main branch was broken? I haven't touched `compiler-rt` at 
> all. Neither anything that deals with the sanitizers. And this fail is 
> "earlier" than the previous one where I indeed missed a test file's update. 
> (Previous fail was at ninja action multiple hundreds, this is 15.)
>
> And it talks about a lot of stuff like `fork` and such that is really really 
> out of scope for the patch itself.

Rule of thumb: If the test failures don't seem related to the contents of your 
patch, they can usually be disregarded as its likely something on trunk that is 
causing the failure.


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

https://reviews.llvm.org/D98635

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

[PATCH] D98635: [libtooling][clang-tidy] Fix diagnostics not respecting and highlighting fed SourceRanges

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@aaron.ballman Could you please help me understand this pre-merge buildbot? It 
says that Debian failed, with the following message:

  [15/1038] Running lint check for sanitizer sources...
  FAILED: projects/compiler-rt/lib/CMakeFiles/SanitizerLintCheck
  cd /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/lib && env 
LLVM_CHECKOUT=/mnt/disks/ssd0/agent/llvm-project/llvm SILENT=1 TMPDIR= 
PYTHON_EXECUTABLE=/usr/bin/python3.8 
COMPILER_RT=/mnt/disks/ssd0/agent/llvm-project/compiler-rt 
/mnt/disks/ssd0/agent/llvm-project/compiler-rt/lib/sanitizer_common/scripts/check_lint.sh
  The following differences between the ABI list and the implemented custom 
wrappers have been found:

Should I be worried because of this, or is this that the patch started building 
when the main branch was broken? I haven't touched `compiler-rt` at all. 
Neither anything that deals with the sanitizers. And this fail is "earlier" 
than the previous one where I indeed missed a test file's update.

And it talks about a lot of stuff like `fork` and such that is really really 
out of scope for the patch itself.


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

https://reviews.llvm.org/D98635

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

[PATCH] D98726: [analyzer] Remove unnecessary TODO

[Balázs Benics via Phabricator via cfe-commits]

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

I don't think the `TODO` is addressed.
By checking the //git blame// quickly, there was no change committed to the 
`SmartPtrChecker` affecting the collaboration with the `MallocChecker` after 
the `TODO` was introduced in the source code.
Thus, I think the `TODO` is probably not yet addressed.

For example this code does not trigger any warning:
https://godbolt.org/z/aM4xe7

  void other(A *oldptr) {
std::unique_ptr P(oldptr);
A* aptr = P.release();
delete aptr;
P->foo(); // No warning here, and probably this is case that the TODO want 
to describe.
  }

Nevertheless, this limitation deserves a test case with a FIXME, if it's not 
there already.




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:16
 #include "SmartPtr.h"
+#include "AllocationState.h"
 

Why do you include this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98726

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

[PATCH] D98635: [libtooling][clang-tidy] Fix diagnostics not respecting and highlighting fed SourceRanges

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Again LGTM, but see what alex says.




Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:135-137
+  for (const FileByteRange  : Error.Message.Ranges) {
+Diag << getRange(FBR);
+  }

nit: Elide braces.



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:279-281
+for (const FileByteRange  : Message.Ranges) {
+  Diag << getRange(FBR);
+}

nit: Elide braces.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:81-84
+  for (const CharSourceRange  : Ranges) {
+Error.Notes.back().Ranges.emplace_back(Loc.getManager(),
+   ToCharRange(SourceRange));
+  }

nit: Elide braces.


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

https://reviews.llvm.org/D98635

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

[PATCH] D89942: Disable LTO and LLD for bootstrap builds on systems unsupported by LLD

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

If you add `include(CMakeDetermineSystem)` to the top of the cache file, it 
should initialize `CMAKE_HOST_SYSTEM_PROCESSOR`, which also probably gives you 
a reasonable value to key off.


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

https://reviews.llvm.org/D89942

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

[PATCH] D97297: [clang-tidy] Suppress reports to patternedly named parameters in 'bugprone-easily-swappable-parameters'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:384
+  // Pad the two strings to the longer length.
+  std::size_t BiggerLength = std::max(Str1.size(), Str2.size());
+  SmallString<32> S1PadE{Str1}, S2PadE{Str2};

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > @aaron.ballman I think I'll do something like `if (BiggerLength < 
> > > Threshold) return false;`, how does that sound? The comparison is moot in 
> > > that case, imagine having strings of 2 and 4, padded to 4, with a 
> > > threshold of 6, for example.
> > > 
> > > That way even if someone accidentally makes `StringRef` overflow the 
> > > buffer, we'll be safe on the call paths we have here.
> > I think that'd make the behavior much more obvious, but should that be `<=`?
> No, because being = to the threshold means that the common prefix/suffix is 
> empty string. It'd make variables such as "A" and "X" deemed related. 
> Generally not something we want, because that's too broad of an assumption 
> that they are "meant to be used together". (Generally you shouldn't name 
> variables like so, but still...)
Ah, that's a good point, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97297

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

[PATCH] D97297: [clang-tidy] Suppress reports to patternedly named parameters in 'bugprone-easily-swappable-parameters'

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:384
+  // Pad the two strings to the longer length.
+  std::size_t BiggerLength = std::max(Str1.size(), Str2.size());
+  SmallString<32> S1PadE{Str1}, S2PadE{Str2};

aaron.ballman wrote:
> whisperity wrote:
> > @aaron.ballman I think I'll do something like `if (BiggerLength < 
> > Threshold) return false;`, how does that sound? The comparison is moot in 
> > that case, imagine having strings of 2 and 4, padded to 4, with a threshold 
> > of 6, for example.
> > 
> > That way even if someone accidentally makes `StringRef` overflow the 
> > buffer, we'll be safe on the call paths we have here.
> I think that'd make the behavior much more obvious, but should that be `<=`?
No, because being = to the threshold means that the common prefix/suffix is 
empty string. It'd make variables such as "A" and "X" deemed related. Generally 
not something we want, because that's too broad of an assumption that they are 
"meant to be used together". (Generally you shouldn't name variables like so, 
but still...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97297

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

[PATCH] D97713: [ASTMatchers] Add documentation for convenience matchers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for this, I think it's helpful documentation! I have a few questions. 
Given that LibASTMatchersReference.html is a generated document, will these 
changes get overwritten by running `dump_ast_matchers.py`? Would it be possible 
to use some form of markup in ASTMatchers.h to generate this documentation so 
that it's easier to flag a particular matcher as being a "convenience" matcher? 
I expect we'll want to extend this list and it'd be nice if we didn't have some 
types of documentation that come from ASTMatchers.h and others that require 
manually editing the generated HTML file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97713

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

[PATCH] D97297: [clang-tidy] Suppress reports to patternedly named parameters in 'bugprone-easily-swappable-parameters'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:384
+  // Pad the two strings to the longer length.
+  std::size_t BiggerLength = std::max(Str1.size(), Str2.size());
+  SmallString<32> S1PadE{Str1}, S2PadE{Str2};

whisperity wrote:
> @aaron.ballman I think I'll do something like `if (BiggerLength < Threshold) 
> return false;`, how does that sound? The comparison is moot in that case, 
> imagine having strings of 2 and 4, padded to 4, with a threshold of 6, for 
> example.
> 
> That way even if someone accidentally makes `StringRef` overflow the buffer, 
> we'll be safe on the call paths we have here.
I think that'd make the behavior much more obvious, but should that be `<=`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97297

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

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I'm really sorry about being sooo picky about this patch.
It's not my expertise and the change seems to address a corner-case, so we have 
to especially careful not introducing bugs.

My concern is that I still don't understand why do we want to do anything with 
reinterpret casts, besides remembering what the original type was.
AFAIK the resulting pointer can not be safely dereferenced unless it's 
reinterpreted back to the original type.
But for example, you are expecting this:

  void testMultiple() {
int F::*f = ::field;
int A::*a = reinterpret_cast(f); // Dereferencing 'a' is UB, AFAIK
int C::*c = reinterpret_cast(f); // Same here!
A aobj;
C cobj;
aobj.*a = 13; // Now an airplane suddenly crashes.
cobj.field = 29;
clang_analyzer_eval(aobj.*a == 13); // expected-warning{{UNKNOWN}} // Same 
here!
clang_analyzer_eval(cobj.*c == 29); // expected-warning{{TRUE}} // Same 
here!
  }

IMO if an expression results in UB, the symbolic value associated with that 
expression should be `Undef`.
`Unknown` would be also fine, but nothing else.

I could spam a couple of nits, but I think it's better to sort this question 
out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

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

[PATCH] D98433: [clang] [C++2b] [P1102] Accept lambdas without parameter list ().

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

That's my question as well to be honest. I don't know what's the policy on that.
Anyway, it will simplify the code a bit I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98433

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

[PATCH] D97297: [clang-tidy] Suppress reports to patternedly named parameters in 'bugprone-easily-swappable-parameters'

[Whisperity via Phabricator via cfe-commits]

whisperity marked an inline comment as not done.
whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:364-365
+StringRef S2) {
+  StringRef S1Prefix = S1.take_front(S1.size() - N),
+S2Prefix = S2.take_front(S2.size() - N);
+  return S1Prefix == S2Prefix && !S1Prefix.empty();

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Should we be checking that `.size() - N` doesn't cause wraparound?
> > Wraparound as in underflow? Like `2 - 8` becoming multiple millions? The 
> > incoming strings are padded to the common length.
> > 
> > `take_xxx` won't let you go out of bounds, even if the parameter is greater 
> > than the string's length. It will just silently return the entire string.
> > Wraparound as in underflow? Like 2 - 8 becoming multiple millions? The 
> > incoming strings are padded to the common length.
> 
> They are, but I didn't see anything checking that `Threshhold` (which gets 
> passed as `N`) isn't larger than the common string length.
> 
> > take_xxx won't let you go out of bounds, even if the parameter is greater 
> > than the string's length. It will just silently return the entire string.
> 
> Aha, that's the important bit, thanks for verifying that this is safe!
Nevertheless I'll clarify this, specifically because I have to make sure (I was 
thinking for a few minutes then realised the `!S1Prefix.empty()` is actually 
making sure of this...) that if you got parameter names of length 1 (let's say) 
and a threshold of 1, it really shouldn't say that `A` and `B` are "related" 
because the common prefix is `""` and the non-common end differs in one 
character, `A` vs. `B`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:384
+  // Pad the two strings to the longer length.
+  std::size_t BiggerLength = std::max(Str1.size(), Str2.size());
+  SmallString<32> S1PadE{Str1}, S2PadE{Str2};

@aaron.ballman I think I'll do something like `if (BiggerLength < Threshold) 
return false;`, how does that sound? The comparison is moot in that case, 
imagine having strings of 2 and 4, padded to 4, with a threshold of 6, for 
example.

That way even if someone accidentally makes `StringRef` overflow the buffer, 
we'll be safe on the call paths we have here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97297

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

[PATCH] D97297: [clang-tidy] Suppress reports to patternedly named parameters in 'bugprone-easily-swappable-parameters'

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:364-365
+StringRef S2) {
+  StringRef S1Prefix = S1.take_front(S1.size() - N),
+S2Prefix = S2.take_front(S2.size() - N);
+  return S1Prefix == S2Prefix && !S1Prefix.empty();

whisperity wrote:
> aaron.ballman wrote:
> > Should we be checking that `.size() - N` doesn't cause wraparound?
> Wraparound as in underflow? Like `2 - 8` becoming multiple millions? The 
> incoming strings are padded to the common length.
> 
> `take_xxx` won't let you go out of bounds, even if the parameter is greater 
> than the string's length. It will just silently return the entire string.
> Wraparound as in underflow? Like 2 - 8 becoming multiple millions? The 
> incoming strings are padded to the common length.

They are, but I didn't see anything checking that `Threshhold` (which gets 
passed as `N`) isn't larger than the common string length.

> take_xxx won't let you go out of bounds, even if the parameter is greater 
> than the string's length. It will just silently return the entire string.

Aha, that's the important bit, thanks for verifying that this is safe!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97297

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

[PATCH] D78652: [clang-tidy] Suppress reports to similarly used parameters in 'bugprone-easily-swappable-parameters'

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:481
+
+  assert(TargetIdx.hasValue() && "Matched, but didn't find index?");
+  TargetParams[PassedParamOfThisFn].insert(

aaron.ballman wrote:
> I *think* you could run into this assert with K C function where there is a 
> `FunctionDecl` to get back to but the decl claims it has no params because it 
> has no prototype (while the call expression actually has the arguments). 
> However, there may be other code that protects us from this case.
At face value, I would say the fact that a K function //has no params// 
declared means that the matcher above will not be able to do 
`forEachArgumentWithParam`, because the argument vector is N long, but the 
parameter vector is empty.

Nevertheless, it won't hurt to add a test case for this though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78652

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

[PATCH] D96355: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with optionally considering differently qualified types mixable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:172
+  /// Add the specified qualifiers to the common type in the Mix.
+  MixData operator<<(Qualifiers Quals) const {
+SplitQualType Split = CommonType.split();

whisperity wrote:
> aaron.ballman wrote:
> > Hmm, use of `<<` is a bit novel but not entirely indefensible. I guess my 
> > initial inclination is that you're combing this information into the mix 
> > data and so an overload of `|` was what I was sort of expecting to see here 
> > (despite it not really being a bitmask operation). These aren't strong 
> > opinions, but I'm curious if you have thoughts.
> I looked at all the possible operator tokens and this seemed the most 
> appropriate. It points to the left, where qualifiers are in LLVM's 
> programming style ("west const" and all that). Because sometimes it's 
> variables that get passed to these functions, seeing from the get-go that 
> it's not meddling with the flags but rather with the types involved seemed 
> appropriate to emphasise.
Okay, that seems reasonable enough to me, thanks!



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:327
+
+  if (LType->isPointerType() && RType->isPointerType()) {
+// If both types are pointers, and pointed to the exact same type,

whisperity wrote:
> aaron.ballman wrote:
> > `isAnyPointerType()` for ObjC pointers? Should we care about rvalue 
> > references here similar to pointers?
> The reasoning to not be special about && goes back to D95736. If you are 
> given any combination of `T`, `T&`, `T&&` and `const T&` parameters and 
> //some// values of (essentially) `T`, it's only `T` and `const T&` that 
> **always** mix for every potential value of `T`.
> 
> `T&&` won't bind variables, and `T&` won't bind temporaries. So their 
> potential to mix is an inherent property of the //call site// where the mix 
> might happen. If one of them is `T` and the other is `T&` and you happen to 
> pass one variable and one literal, and you happen to swap these two, you'll 
> get a compile error.
> 
> If both parameters are `T&&`, `LType == RType` for a trivial mix catches it 
> early on.
Ah, thank you for the explanation about rvalue references, that's helpful!

As for ObjC... after thinking about it a bit more, I'm on the fence. On the one 
hand, ObjC is C + extra features, and you can swap parameters in C. On the 
other hand, ObjC tends to use message sending an awful lot where names the 
parameters are used at the call site, so this check is less interesting in an 
ObjC code base in some ways. I think it might make more sense to ignore ObjC 
for now and try to tackle that in the future (perhaps at user request).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Sorry for being unresponsive for a while, I got distracted by various bugs.

I skimmed this and it's looking great. Just added a few nit picks.




Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:32
+  // If lookup table has more than one user,
+  // do not generate a relative lookup table.
+  if (!GV.hasOneUse())

It would be better if the comment said why. I suppose the reason is we need to 
be sure there aren't other uses of the table, because then it can't be replaced.

But it would be cool if a future version of this patch could handle when there 
are multiple loads from the table which can all be replaced -- for example this 
could happen if a function which uses a lookup table gets inlined into multiple 
places.



Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:47
+
+  // If the original lookup table is not dso_local,
+  // do not generate a relative lookup table.

It would be good with a "why" here too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D98726: [analyzer] Remove unnecessary TODO

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD created this revision.
RedDocMD added reviewers: NoQ, vsavchenko, steakhal.
Herald added subscribers: ASDenysPetrov, martong, Charusso, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
RedDocMD requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This I think should not be a TODO, since MallocChecker indeed does track
released pointers. I have added a test for that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98726

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp


Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  
-analyzer-checker=core,cplusplus.Move,cplusplus.NewDelete,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
@@ -43,6 +43,14 @@
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
+void derefOfReleasedPtr(A *oldptr) {
+  std::unique_ptr P(oldptr);
+  A* aptr = P.release();
+  delete aptr; // expected-note {{Memory is released}}
+  aptr->foo(); // expected-warning {{Use of memory after it is freed 
[cplusplus.NewDelete]}}
+  // expected-note@-1{{Use of memory after it is freed}}
+}
+
 void derefAfterReset() {
   std::unique_ptr P(new A()); // expected-note {{Smart pointer 'P' is 
constructed}}
   P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -13,6 +13,7 @@
 
 #include "Move.h"
 #include "SmartPtr.h"
+#include "AllocationState.h"
 
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
@@ -391,8 +392,6 @@
 checkAndPrettyPrintRegion(OS, ThisRegion);
 OS << " is released and set to null";
   }));
-  // TODO: Add support to enable MallocChecker to start tracking the raw
-  // pointer.
 }
 
 void SmartPtrModeling::handleSwap(const CallEvent ,


Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,cplusplus.NewDelete,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
@@ -43,6 +43,14 @@
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
+void derefOfReleasedPtr(A *oldptr) {
+  std::unique_ptr P(oldptr);
+  A* aptr = P.release();
+  delete aptr; // expected-note {{Memory is released}}
+  aptr->foo(); // expected-warning {{Use of memory after it is freed [cplusplus.NewDelete]}}
+  // expected-note@-1{{Use of memory after it is freed}}
+}
+
 void derefAfterReset() {
   std::unique_ptr P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
   P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -13,6 +13,7 @@
 
 #include "Move.h"
 #include "SmartPtr.h"
+#include "AllocationState.h"
 
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
@@ -391,8 +392,6 @@
 checkAndPrettyPrintRegion(OS, ThisRegion);
 OS << " is released and set to null";
   }));
-  // TODO: Add support to enable MallocChecker to start tracking the raw
-  // pointer.
 }
 
 void SmartPtrModeling::handleSwap(const CallEvent ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95736: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with `typedef` and `const &` diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:706
+UniqueBindPower({LType, RType})) {
+  // FIXME: Don't emit multiple combinations here either.
+  StringRef DiagText = "a call site binds an expression to '%0' and "

whisperity wrote:
> This is a stale FIXME, the uniqueing of the emission is in the line right 
> above...
Heh, I was sort of wondering about that. Please remove the FIXME before 
committing.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:707-708
+  // FIXME: Don't emit multiple combinations here either.
+  StringRef DiagText = "a call site binds an expression to '%0' and "
+   "'%1' with the same force";
+  diag(M.Second->getOuterLocStart(), DiagText, DiagnosticIDs::Note)

whisperity wrote:
> aaron.ballman wrote:
> > I think "with the same force" is going to be hard for users to make sense 
> > of. I'm at a bit of a loss for how to word it though. The issue is that `T` 
> > and `const T&` parameters *might* be easily swapped, so maybe it's best to 
> > call it out that way?
> This is a note tag to explain the reason behind the mix. The warning itself 
> has been emitted before. How about
> 
> > a `T` and `const T&` parameter accepts all values of `T`
> 
> ?
I think that'd be significantly more clear, yes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95736

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

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69560#2629616 , @whisperity wrote:

> In D69560#2629555 , @aaron.ballman 
> wrote:
>
>> [...] so please hold off on landing it for a bit in case any of the other 
>> reviewers want to weigh in.
>
> Due to how the patch itself only being useful in practice if all the pieces 
> fall into place, I will definitely keep circling about until the **entire** 
> patch tree is ✔. (Including the filtering heuristics, etc.)

Good idea. :-)




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > Are you planning to remove the debugging code now that the check is 
> > > > approaching its final form?
> > > Actually, I would prefer not to. I removed every debug thing possible. 
> > > However, and this is speaking from experience because I wrote this check 
> > > two times already from basically scratch... the rest of the debug code 
> > > that is part of the patch has to be there. If anything goes nuts, 
> > > especially if there would be false positives later... it would be 
> > > impossible to figure out what is going on during the modelling without 
> > > seeing all the steps being taken.
> > We don't often leave debugging statements in because they tend to cause a 
> > maintenance burden that doesn't justify their benefit. However, I agree 
> > with your observation that this code is quite difficult to reason through 
> > without debugging aids.
> > 
> > I don't insist on removing the code yet, but would say we should remain 
> > open to the idea if it causes a burden in practice. (Either in terms of 
> > keeping the debugging code up to date as the check evolves, but also in 
> > terms of compile time performance for the check if the debugging code paths 
> > turn out to be expensive on build times.)
> All debugging code is wrapped into the `LLVM_DEBUG` macro specifically 
> (that's why I even put this little "print bits" function behind `NDEBUG`) so 
> they are not part of the builds.
> 
> I think in general //if// someone puts the effort into reading the code and 
> has an interactive debugger they could figure it out (especially if they keep 
> track of the recursion constantly), I tried to make everything nicely padded 
> and all the variable names and control flow to make sense. (Of course the 
> wealth of complexity comes in the follow-up patches.)
> All debugging code is wrapped into the LLVM_DEBUG macro specifically (that's 
> why I even put this little "print bits" function behind NDEBUG) so they are 
> not part of the builds.

They're part of debug builds still (which is the build configuration I tend to 
use most often). That said, I think it's fine to leave the debugging code in 
for now, especially as the check is being actively worked on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560

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

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D69560#2629555 , @aaron.ballman 
wrote:

> [...] so please hold off on landing it for a bit in case any of the other 
> reviewers want to weigh in.

Due to how the patch itself only being useful in practice if all the pieces 
fall into place, I will definitely keep circling about until the **entire** 
patch tree is ✔. (Including the filtering heuristics, etc.)




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Are you planning to remove the debugging code now that the check is 
> > > approaching its final form?
> > Actually, I would prefer not to. I removed every debug thing possible. 
> > However, and this is speaking from experience because I wrote this check 
> > two times already from basically scratch... the rest of the debug code that 
> > is part of the patch has to be there. If anything goes nuts, especially if 
> > there would be false positives later... it would be impossible to figure 
> > out what is going on during the modelling without seeing all the steps 
> > being taken.
> We don't often leave debugging statements in because they tend to cause a 
> maintenance burden that doesn't justify their benefit. However, I agree with 
> your observation that this code is quite difficult to reason through without 
> debugging aids.
> 
> I don't insist on removing the code yet, but would say we should remain open 
> to the idea if it causes a burden in practice. (Either in terms of keeping 
> the debugging code up to date as the check evolves, but also in terms of 
> compile time performance for the check if the debugging code paths turn out 
> to be expensive on build times.)
All debugging code is wrapped into the `LLVM_DEBUG` macro specifically (that's 
why I even put this little "print bits" function behind `NDEBUG`) so they are 
not part of the builds.

I think in general //if// someone puts the effort into reading the code and has 
an interactive debugger they could figure it out (especially if they keep track 
of the recursion constantly), I tried to make everything nicely padded and all 
the variable names and control flow to make sense. (Of course the wealth of 
complexity comes in the follow-up patches.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560

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

[PATCH] D98724: Fix false negative in -Wthread-safety-attributes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added a reviewer: aaron.ballman.
aaronpuchert requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The original implementation didn't fire on non-template classes when a
base class was an instantiation of a template with a dependent base.
In that case the base of the base is dependent as seen from the base,
but not from the class we're interested in, which isn't a template.

Also it simplifies the code a lot.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98724

Files:
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaCXX/warn-thread-safety-parsing.cpp


Index: clang/test/SemaCXX/warn-thread-safety-parsing.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -1295,6 +1295,11 @@
 // expected-warning{{'unlock_function' attribute without capability 
arguments refers to 'this', but 'SLDerived2' isn't annotated with 'capability' 
or 'scoped_lockable' attribute}}
 };
 
+struct SLDerived3 : public SLTemplateDerived {
+  ~SLDerived3() UNLOCK_FUNCTION(); // \
+// expected-warning{{'unlock_function' attribute without capability 
arguments refers to 'this', but 'SLDerived3' isn't annotated with 'capability' 
or 'scoped_lockable' attribute}}
+};
+
 //-
 // Parsing of member variables and function parameters
 //--
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -513,16 +513,9 @@
 
   // Else check if any base classes have the attribute.
   if (const auto *CRD = dyn_cast(RD)) {
-CXXBasePaths BPaths(false, false);
-if (CRD->lookupInBases(
-[](const CXXBaseSpecifier *BS, CXXBasePath &) {
-  const auto  = *BS->getType();
-  // If it's type-dependent, we assume it could have the attribute.
-  if (Ty.isDependentType())
-return true;
-  return Ty.castAs()->getDecl()->hasAttr();
-},
-BPaths, true))
+if (!CRD->forallBases([](const CXXRecordDecl *Base) {
+  return !Base->hasAttr();
+}))
   return true;
   }
   return false;


Index: clang/test/SemaCXX/warn-thread-safety-parsing.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -1295,6 +1295,11 @@
 // expected-warning{{'unlock_function' attribute without capability arguments refers to 'this', but 'SLDerived2' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
+struct SLDerived3 : public SLTemplateDerived {
+  ~SLDerived3() UNLOCK_FUNCTION(); // \
+// expected-warning{{'unlock_function' attribute without capability arguments refers to 'this', but 'SLDerived3' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
+};
+
 //-
 // Parsing of member variables and function parameters
 //--
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -513,16 +513,9 @@
 
   // Else check if any base classes have the attribute.
   if (const auto *CRD = dyn_cast(RD)) {
-CXXBasePaths BPaths(false, false);
-if (CRD->lookupInBases(
-[](const CXXBaseSpecifier *BS, CXXBasePath &) {
-  const auto  = *BS->getType();
-  // If it's type-dependent, we assume it could have the attribute.
-  if (Ty.isDependentType())
-return true;
-  return Ty.castAs()->getDecl()->hasAttr();
-},
-BPaths, true))
+if (!CRD->forallBases([](const CXXRecordDecl *Base) {
+  return !Base->hasAttr();
+}))
   return true;
   }
   return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97297: [clang-tidy] Suppress reports to patternedly named parameters in 'bugprone-easily-swappable-parameters'

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:364-365
+StringRef S2) {
+  StringRef S1Prefix = S1.take_front(S1.size() - N),
+S2Prefix = S2.take_front(S2.size() - N);
+  return S1Prefix == S2Prefix && !S1Prefix.empty();

aaron.ballman wrote:
> Should we be checking that `.size() - N` doesn't cause wraparound?
Wraparound as in underflow? Like `2 - 8` becoming multiple millions? The 
incoming strings are padded to the common length.

`take_xxx` won't let you go out of bounds, even if the parameter is greater 
than the string's length. It will just silently return the entire string.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97297

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

[PATCH] D98712: [Utils] Support lit-like substitutions in update_cc_test_checks

[Giorgis Georgakoudis 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 rGa80a33e8b553: [Utils] Support lit-like substitutions in 
update_cc_test_checks (authored by ggeorgakoudis).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98712

Files:
  clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
  clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
  llvm/utils/update_cc_test_checks.py


Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -176,7 +176,7 @@
   return args, parser
 
 
-def get_function_body(builder, args, filename, clang_args, extra_commands, 
+def get_function_body(builder, args, filename, clang_args, extra_commands,
   prefixes):
   # TODO Clean up duplication of asm/common build_function_body_dictionary
   # Invoke external tool and extract function bodies.
@@ -221,6 +221,13 @@
 # Build a list of clang command lines and check prefixes from RUN lines.
 run_list = []
 line2spell_and_mangled_list = collections.defaultdict(list)
+
+subs = {
+  '%s' : ti.path,
+  '%t' : tempfile.NamedTemporaryFile().name,
+  '%S' : os.getcwd(),
+}
+
 for l in ti.run_lines:
   commands = [cmd.strip() for cmd in l.split('|')]
 
@@ -234,15 +241,18 @@
   # Execute non-clang runline.
   if exec_args[0] not in SUBST:
 print('NOTE: Executing non-clang RUN line: ' + l, file=sys.stderr)
-# Replace %s by `filename`.
-exec_args = [i.replace('%s', ti.path) if '%s' in i else i for i in 
exec_args]
+# Do lit-like substitutions.
+for s in subs:
+  exec_args = [i.replace(s, subs[s]) if s in i else i for i in 
exec_args]
 exec_run_line(exec_args)
 continue
-  # This is a clang runline, apply %clang substitution rule, replace %s by 
`filename`,
+  # This is a clang runline, apply %clang substitution rule, do lit-like 
substitutions,
   # and append args.clang_args
   clang_args = exec_args
   clang_args[0:1] = SUBST[clang_args[0]]
-  clang_args = [i.replace('%s', ti.path) if '%s' in i else i for i in 
clang_args] + ti.args.clang_args
+  for s in subs:
+clang_args = [i.replace(s, subs[s]) if s in i else i for i in 
clang_args]
+  clang_args += ti.args.clang_args
 
   # Extract -check-prefix in FileCheck args
   filecheck_cmd = commands[-1]
@@ -271,7 +281,7 @@
   common.debug('Extracted clang cmd: clang {}'.format(clang_args))
   common.debug('Extracted FileCheck prefixes: {}'.format(prefixes))
 
-  get_function_body(builder, ti.args, ti.path, clang_args, extra_commands, 
+  get_function_body(builder, ti.args, ti.path, clang_args, extra_commands,
 prefixes)
 
   # Invoke clang -Xclang -ast-dump=json to get mapping from start lines to
@@ -315,7 +325,7 @@
  prefixes,
  func_dict, func)
 
-  common.add_checks_at_end(output_lines, run_list, builder.func_order(), 
+  common.add_checks_at_end(output_lines, run_list, builder.func_order(),
'//', lambda my_output_lines, prefixes, func:
check_generator(my_output_lines,
prefixes, func))
Index: 
clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
===
--- clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
+++ clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
@@ -1,6 +1,7 @@
 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // Check that the non-clang/non-filechecked runlines execute
-// RUN: cp %s %s.copy.c
+// RUN: cp %s %S/Output/execute-all-runlines.copy.c
+// RUN: cp %S/Output/execute-all-runlines.copy.c %s.copy.c
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp %s.copy.c 
-emit-llvm-bc -o %t-host.bc
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp 
-fopenmp-host-ir-file-path %t-host.bc %s.copy.c -emit-llvm -o - | FileCheck %s
 
Index: clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
===
--- clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
+++ clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
@@ -1,5 +1,6 @@
 // Check that the non-clang/non-filechecked runlines execute
-// RUN: cp %s %s.copy.c
+// RUN: cp %s %S/Output/execute-all-runlines.copy.c
+// RUN: cp %S/Output/execute-all-runlines.copy.c %s.copy.c
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu 

[clang] a80a33e - [Utils] Support lit-like substitutions in update_cc_test_checks

[Giorgis Georgakoudis via cfe-commits]

Author: Giorgis Georgakoudis
Date: 2021-03-16T10:36:22-07:00
New Revision: a80a33e8b55393c060e51486cfd8085b380eb36d

URL: 
https://github.com/llvm/llvm-project/commit/a80a33e8b55393c060e51486cfd8085b380eb36d
DIFF: 
https://github.com/llvm/llvm-project/commit/a80a33e8b55393c060e51486cfd8085b380eb36d.diff

LOG: [Utils] Support lit-like substitutions in update_cc_test_checks

Reviewed By: jdoerfert

Differential Revision: https://reviews.llvm.org/D98712

Added: 


Modified: 
clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
llvm/utils/update_cc_test_checks.py

Removed: 




diff  --git a/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c 
b/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
index 1626eb540841..e0dfc42c4bd6 100644
--- a/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
+++ b/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
@@ -1,5 +1,6 @@
 // Check that the non-clang/non-filechecked runlines execute
-// RUN: cp %s %s.copy.c
+// RUN: cp %s %S/Output/execute-all-runlines.copy.c
+// RUN: cp %S/Output/execute-all-runlines.copy.c %s.copy.c
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp %s.copy.c 
-emit-llvm-bc -o %t-host.bc
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp 
-fopenmp-host-ir-file-path %t-host.bc %s.copy.c -emit-llvm -o - | FileCheck %s
 

diff  --git 
a/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected 
b/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
index 5edf11e668e4..ae9745fa9b1e 100644
--- a/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
+++ b/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
@@ -1,6 +1,7 @@
 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // Check that the non-clang/non-filechecked runlines execute
-// RUN: cp %s %s.copy.c
+// RUN: cp %s %S/Output/execute-all-runlines.copy.c
+// RUN: cp %S/Output/execute-all-runlines.copy.c %s.copy.c
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp %s.copy.c 
-emit-llvm-bc -o %t-host.bc
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp 
-fopenmp-host-ir-file-path %t-host.bc %s.copy.c -emit-llvm -o - | FileCheck %s
 

diff  --git a/llvm/utils/update_cc_test_checks.py 
b/llvm/utils/update_cc_test_checks.py
index d084bc6d0795..d3af5308ac6a 100755
--- a/llvm/utils/update_cc_test_checks.py
+++ b/llvm/utils/update_cc_test_checks.py
@@ -176,7 +176,7 @@ def config():
   return args, parser
 
 
-def get_function_body(builder, args, filename, clang_args, extra_commands, 
+def get_function_body(builder, args, filename, clang_args, extra_commands,
   prefixes):
   # TODO Clean up duplication of asm/common build_function_body_dictionary
   # Invoke external tool and extract function bodies.
@@ -221,6 +221,13 @@ def main():
 # Build a list of clang command lines and check prefixes from RUN lines.
 run_list = []
 line2spell_and_mangled_list = collections.defaultdict(list)
+
+subs = {
+  '%s' : ti.path,
+  '%t' : tempfile.NamedTemporaryFile().name,
+  '%S' : os.getcwd(),
+}
+
 for l in ti.run_lines:
   commands = [cmd.strip() for cmd in l.split('|')]
 
@@ -234,15 +241,18 @@ def main():
   # Execute non-clang runline.
   if exec_args[0] not in SUBST:
 print('NOTE: Executing non-clang RUN line: ' + l, file=sys.stderr)
-# Replace %s by `filename`.
-exec_args = [i.replace('%s', ti.path) if '%s' in i else i for i in 
exec_args]
+# Do lit-like substitutions.
+for s in subs:
+  exec_args = [i.replace(s, subs[s]) if s in i else i for i in 
exec_args]
 exec_run_line(exec_args)
 continue
-  # This is a clang runline, apply %clang substitution rule, replace %s by 
`filename`,
+  # This is a clang runline, apply %clang substitution rule, do lit-like 
substitutions,
   # and append args.clang_args
   clang_args = exec_args
   clang_args[0:1] = SUBST[clang_args[0]]
-  clang_args = [i.replace('%s', ti.path) if '%s' in i else i for i in 
clang_args] + ti.args.clang_args
+  for s in subs:
+clang_args = [i.replace(s, subs[s]) if s in i else i for i in 
clang_args]
+  clang_args += ti.args.clang_args
 
   # Extract -check-prefix in FileCheck args
   filecheck_cmd = commands[-1]
@@ -271,7 +281,7 @@ def main():
   common.debug('Extracted clang cmd: clang {}'.format(clang_args))
   common.debug('Extracted FileCheck prefixes: {}'.format(prefixes))
 
-  get_function_body(builder, ti.args, ti.path, clang_args, extra_commands, 
+  get_function_body(builder, ti.args, ti.path, clang_args, extra_commands,
 prefixes)
 
 

[PATCH] D96355: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with optionally considering differently qualified types mixable

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:172
+  /// Add the specified qualifiers to the common type in the Mix.
+  MixData operator<<(Qualifiers Quals) const {
+SplitQualType Split = CommonType.split();

aaron.ballman wrote:
> Hmm, use of `<<` is a bit novel but not entirely indefensible. I guess my 
> initial inclination is that you're combing this information into the mix data 
> and so an overload of `|` was what I was sort of expecting to see here 
> (despite it not really being a bitmask operation). These aren't strong 
> opinions, but I'm curious if you have thoughts.
I looked at all the possible operator tokens and this seemed the most 
appropriate. It points to the left, where qualifiers are in LLVM's programming 
style ("west const" and all that). Because sometimes it's variables that get 
passed to these functions, seeing from the get-go that it's not meddling with 
the flags but rather with the types involved seemed appropriate to emphasise.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:327
+
+  if (LType->isPointerType() && RType->isPointerType()) {
+// If both types are pointers, and pointed to the exact same type,

aaron.ballman wrote:
> `isAnyPointerType()` for ObjC pointers? Should we care about rvalue 
> references here similar to pointers?
The reasoning to not be special about && goes back to D95736. If you are given 
any combination of `T`, `T&`, `T&&` and `const T&` parameters and //some// 
values of (essentially) `T`, it's only `T` and `const T&` that **always** mix 
for every potential value of `T`.

`T&&` won't bind variables, and `T&` won't bind temporaries. So their potential 
to mix is an inherent property of the //call site// where the mix might happen. 
If one of them is `T` and the other is `T&` and you happen to pass one variable 
and one literal, and you happen to swap these two, you'll get a compile error.

If both parameters are `T&&`, `LType == RType` for a trivial mix catches it 
early on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96355

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

[PATCH] D98692: [clang-tidy] Add cppcoreguidelines-comparison-operator

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp:20-21
+void ComparisonOperatorCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(functionDecl(hasAnyOverloadedOperatorName("==", "!=", "<",
+   "<=", ">", 
">="))
+ .bind("operator"),

Is there a precedent to match spaceship operator?



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp:48-52
+  if (isa(Operator)) {
+diag(Operator->getLocation(), "%0 should not be member function")
+<< Operator;
+return;
+  }

We should likely only warn on the canonical declaration
```lang=c++
struct A{
  bool operator==(const A&) const; // Warn here
};

bool A::operator==(const A&) const { // Don't warn here
  return true;
}```



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp:54-55
+
+  if (canThrow(Operator))
+diagCanThrow(Operator);
+

This code should not be executed when running in pre c++11 mode, as noexcept 
was added in c++11.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-comparison-operator.cpp:1
+// RUN: %check_clang_tidy %s cppcoreguidelines-comparison-operator %t
+

Once you rebase to trunk this test will fail without `--fix-notes` specified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98692

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

[PATCH] D98712: [Utils] Support lit-like substitutions in update_cc_test_checks

[Giorgis Georgakoudis via Phabricator via cfe-commits]

ggeorgakoudis updated this revision to Diff 331037.
ggeorgakoudis added a comment.

Update test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98712

Files:
  clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
  clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
  llvm/utils/update_cc_test_checks.py


Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -176,7 +176,7 @@
   return args, parser
 
 
-def get_function_body(builder, args, filename, clang_args, extra_commands, 
+def get_function_body(builder, args, filename, clang_args, extra_commands,
   prefixes):
   # TODO Clean up duplication of asm/common build_function_body_dictionary
   # Invoke external tool and extract function bodies.
@@ -221,6 +221,13 @@
 # Build a list of clang command lines and check prefixes from RUN lines.
 run_list = []
 line2spell_and_mangled_list = collections.defaultdict(list)
+
+subs = {
+  '%s' : ti.path,
+  '%t' : tempfile.NamedTemporaryFile().name,
+  '%S' : os.getcwd(),
+}
+
 for l in ti.run_lines:
   commands = [cmd.strip() for cmd in l.split('|')]
 
@@ -234,15 +241,18 @@
   # Execute non-clang runline.
   if exec_args[0] not in SUBST:
 print('NOTE: Executing non-clang RUN line: ' + l, file=sys.stderr)
-# Replace %s by `filename`.
-exec_args = [i.replace('%s', ti.path) if '%s' in i else i for i in 
exec_args]
+# Do lit-like substitutions.
+for s in subs:
+  exec_args = [i.replace(s, subs[s]) if s in i else i for i in 
exec_args]
 exec_run_line(exec_args)
 continue
-  # This is a clang runline, apply %clang substitution rule, replace %s by 
`filename`,
+  # This is a clang runline, apply %clang substitution rule, do lit-like 
substitutions,
   # and append args.clang_args
   clang_args = exec_args
   clang_args[0:1] = SUBST[clang_args[0]]
-  clang_args = [i.replace('%s', ti.path) if '%s' in i else i for i in 
clang_args] + ti.args.clang_args
+  for s in subs:
+clang_args = [i.replace(s, subs[s]) if s in i else i for i in 
clang_args]
+  clang_args += ti.args.clang_args
 
   # Extract -check-prefix in FileCheck args
   filecheck_cmd = commands[-1]
@@ -271,7 +281,7 @@
   common.debug('Extracted clang cmd: clang {}'.format(clang_args))
   common.debug('Extracted FileCheck prefixes: {}'.format(prefixes))
 
-  get_function_body(builder, ti.args, ti.path, clang_args, extra_commands, 
+  get_function_body(builder, ti.args, ti.path, clang_args, extra_commands,
 prefixes)
 
   # Invoke clang -Xclang -ast-dump=json to get mapping from start lines to
@@ -315,7 +325,7 @@
  prefixes,
  func_dict, func)
 
-  common.add_checks_at_end(output_lines, run_list, builder.func_order(), 
+  common.add_checks_at_end(output_lines, run_list, builder.func_order(),
'//', lambda my_output_lines, prefixes, func:
check_generator(my_output_lines,
prefixes, func))
Index: 
clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
===
--- clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
+++ clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
@@ -1,6 +1,7 @@
 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // Check that the non-clang/non-filechecked runlines execute
-// RUN: cp %s %s.copy.c
+// RUN: cp %s %S/Output/execute-all-runlines.copy.c
+// RUN: cp %S/Output/execute-all-runlines.copy.c %s.copy.c
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp %s.copy.c 
-emit-llvm-bc -o %t-host.bc
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp 
-fopenmp-host-ir-file-path %t-host.bc %s.copy.c -emit-llvm -o - | FileCheck %s
 
Index: clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
===
--- clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
+++ clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
@@ -1,5 +1,6 @@
 // Check that the non-clang/non-filechecked runlines execute
-// RUN: cp %s %s.copy.c
+// RUN: cp %s %S/Output/execute-all-runlines.copy.c
+// RUN: cp %S/Output/execute-all-runlines.copy.c %s.copy.c
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp %s.copy.c 
-emit-llvm-bc -o %t-host.bc
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fopenmp 
-fopenmp-host-ir-file-path %t-host.bc %s.copy.c 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

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

In general, I'm happy with this check (and the follow-ups), so I'm giving it my 
LG. However, it's complex enough that I think having some extra review from 
@alexfh, @hokein, or one of the other reviewers would be a good idea, so please 
hold off on landing it for a bit in case any of the other reviewers want to 
weigh in.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+

whisperity wrote:
> aaron.ballman wrote:
> > Are you planning to remove the debugging code now that the check is 
> > approaching its final form?
> Actually, I would prefer not to. I removed every debug thing possible. 
> However, and this is speaking from experience because I wrote this check two 
> times already from basically scratch... the rest of the debug code that is 
> part of the patch has to be there. If anything goes nuts, especially if there 
> would be false positives later... it would be impossible to figure out what 
> is going on during the modelling without seeing all the steps being taken.
We don't often leave debugging statements in because they tend to cause a 
maintenance burden that doesn't justify their benefit. However, I agree with 
your observation that this code is quite difficult to reason through without 
debugging aids.

I don't insist on removing the code yet, but would say we should remain open to 
the idea if it causes a burden in practice. (Either in terms of keeping the 
debugging code up to date as the check evolves, but also in terms of compile 
time performance for the check if the debugging code paths turn out to be 
expensive on build times.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560

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