[PATCH] D159355: [clang][dataflow] Unsoundly treat "Unknown" as "Equivalent" in widening.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

Sorry for the late review. This looks good to me, but I hope we will be able to 
undo it soon :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159355

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

[PATCH] D159109: [analyzer] CStringChecker buffer access checks should check the first bytes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

LG!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159109

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

[PATCH] D159108: [analyzer] CStringChecker should check the first byte of the destination of strcpy, strncpy

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

LG!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159108

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

[PATCH] D158977: [clang][dataflow] Don't associate prvalue expressions with storage locations.

[Gábor Horváth via Phabricator via cfe-commits]

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

Thanks! Sometimes I am wondering whether we actually need a full map for 
PRValues. E.g., once we processed a `MaterializeTemporaryExpr`, we now have a 
location for the value, and it feels like we represent the same thing twice, 
once in `ExprToLoc + LocToVal` and once in `ExprToVal`. It is probably not too 
bad and might be extra work to clean this up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158977

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

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp:27
   class BoolAssignmentChecker : public Checker< check::Bind > {
-mutable std::unique_ptr BT;
+mutable std::unique_ptr BT;
 void emitReport(ProgramStateRef state, CheckerContext ,

In case you are down to some additional cleanup efforts, we no longer need lazy 
initialization for `BugType`s. This is an old pattern, we used to need it due 
to some initialization order problems, but those have been worked around in 
engine. See the FuchsiaHandleChecker as an example how we do it in newer 
checks: 
https://github.com/llvm/llvm-project/blob/ea82a822d990824c58c6fa4b503ca84c4870c940/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp#L190

Feel free to ignore this or do it in a follow-up PR. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

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

[PATCH] D156658: [clang][dataflow] When checking `ExprToLoc` convergence, only consider children of block terminator.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D156658#4606400 , @mboehme wrote:

> We only do widening at loop heads, and this means that widening only affects 
> locations and values that flow into the loop from the outside or from a 
> previous loop iteration.
>
> But convergence can also be blocked by locations and values that are only 
> used within the loop body. If these change from loop iteration to loop 
> iteration and we don't perform widening on them, we will conclude that the 
> state of the loop body never converges.

I wonder if this is something we need to solve. Maybe when we do the widening 
at the loop head that should also affect values that are not flowing into that 
node. I.e., at the loop heads we might want to also look at the values from the 
previous iteration that are only used within the loop body.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156658

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

[PATCH] D153071: [clang][dataflow] Refactor `DataflowWorklist` types and add a WTO version.

[Gábor Horváth via Phabricator via cfe-commits]

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

Mea culpa. Looks like I did not anticipate non-RPO worklists. Thanks for 
cleaning this up, looks good to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153071

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

[PATCH] D156658: [clang][dataflow] When checking `ExprToLoc` convergence, only consider children of block terminator.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D156658#4552965 , @mboehme wrote:

> I've investigated this in more detail. Unfortunately, it turns out that it's 
> not quite as simple as just implementing widening on `ExprToLoc`.
>
> One of the reasons for this is that we only apply widening at loop heads, but 
> the expressions that are "blocking" convergence may be contained in a block 
> that is not a loop head.

I am probably missing something, but I why does it matter where are we doing 
the widening? It is possible that we might need to redesign how parts of the 
program state is represented, but I do not immediately see any fundamental 
roadblocks.

> I think the real solution to this would be to introduce widening for 
> `PointerValue`s.

Fully agreed.

> Essentially, what we want is a "top" `PointerValue` that does not have an 
> associated `StorageLocation`. However, we don't want to eliminate the 
> `PointerValue` entirely; we still want to be able to attach properties to it, 
> so that, for example, an analysis can record that the `PointerValue` is 
> non-null, even though we don't know what its exact location is.

Another way to interpret "top": it points to a "summary" `StorageLocation` that 
can be any other `StorageLocation`, we just do not know which one. This 
interpretation/formulation has some advantages:

- We have a `StorageLocation` to use when we dereference these top pointers.
- It is compatible with the alias sets representation.
- It is compatible with some other representations where we have other 
"summary" locations, like "UnkownStackLocation" or "UnkownHeapLocation".

These summary memory locations are sort of the union of all the potential 
memory locations they could represent. I think in general it might be useful to 
embrace this idea, e.g., when we model arrays, we can have a single element 
region representing all the knowledge we know to be true for all elements of 
the array.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156658

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

[PATCH] D156658: [clang][dataflow] When checking `ExprToLoc` convergence, only consider children of block terminator.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

> It looks as if, instead, what we should be doing to improve convergence in a 
> sound manner is to implement widening for ExprToLoc. I'll submit a 
> corresponding patch shortly.

+1, I believe we want `ExprToLoc` to converge. That being said, if we can get 
away with not checking some parts, it could potentially be implemented as an 
optimization. In that case, I'd expect to still have full checking in debug 
builds and a strong argument why is it OK to not compare some parts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156658

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

[PATCH] D155446: [clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:201
 
-/// Models a value of `struct` or `class` type, with a flat map of fields to
-/// child storage locations, containing all accessible members of base struct
-/// and class types.
+/// Models a value of `struct` or `class` type.
+/// In C++, prvalues of class type serve only a limited purpose: They can only

mboehme wrote:
> xazax.hun wrote:
> > Should we link to the design doc somewhere? The comments are great and 
> > self-contained, but I wonder if there is still some value for the doc to be 
> > mentioned somewhere in the code. I do not have a preference, just 
> > wondering. 
> I'm hestitant do to this, as the doc is likely to become out of date at some 
> point. We've already discussed plans on another patch to eliminate 
> `StructValue` entirely, for example. So I'd rather make sure we have 
> sufficient documentation with the code itself, where it's most likely to be 
> kept up to date.
> 
> Anything in particular from the doc that you think would be worth capturing 
> here? (I intend to submit shortly, but would address this in a followup 
> patch.)
I have nothing I would miss from the comments, but there were more code 
examples in the design doc that helped me understand the problem. I can imagine 
some people have easier time consuming that document for this reason. But I 
agree that this is not a strong enough argument to link to something that might 
go stale in the future. 



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:534-549
 void transferCallReturningOptional(const CallExpr *E,
const MatchFinder::MatchResult ,
LatticeTransferState ) {
   if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
 return;
 
+  AggregateStorageLocation *Loc = nullptr;

mboehme wrote:
> xazax.hun wrote:
> > This is more of a note for the future of these APIs. I wonder if this is 
> > the right level of abstraction in this framework. I think it would be great 
> > to have something like:
> > ```
> > void transferCallReturningOptional(const CallExpr *E,
> >StorageLocation* RetLoc,
> >ArrayRef Args,
> >const MatchFinder::MatchResult ,
> >LatticeTransferState );
> > ```
> > 
> > and all the values and storage locations could come from the framework. It 
> > would be great if the user would almost never need to get a value or a 
> > location from an expression, they would be readily available. What do you 
> > think?
> Hm, interesting idea! This definitely sounds like a way to avoid mistakes, 
> though there would definitely be some details to work out -- for example, a 
> `StorageLocation` for the arguments only works if they are passed by 
> reference, not by value. Something to think about for sure though.
Another potential advantage of that approach would be to amortize the lookup 
costs in case there are multiple checks participating in the same fixed-point 
iteration. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155446

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

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

[Gábor Horváth via Phabricator via cfe-commits]

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

Thanks, looks good to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058

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

[PATCH] D155921: [clang][dataflow] Reverse course on `getValue()` deprecation.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

Sounds good to me. I believe this make check author's lives easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155921

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

[PATCH] D155847: [analyzer] Fix crash in GenericTaintChecker when propagatig taint to AllocaRegion

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155847

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

[PATCH] D155446: [clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

I did not do a thorough review checking every line, but I read the design paper 
and skimmed through this patch. Love the direction, and I am OK with landing 
this as is.




Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:677
-  // them and the decls of the struct members.
-  llvm::DenseMap>

I am glad to see this gone.



Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:201
 
-/// Models a value of `struct` or `class` type, with a flat map of fields to
-/// child storage locations, containing all accessible members of base struct
-/// and class types.
+/// Models a value of `struct` or `class` type.
+/// In C++, prvalues of class type serve only a limited purpose: They can only

Should we link to the design doc somewhere? The comments are great and 
self-contained, but I wonder if there is still some value for the doc to be 
mentioned somewhere in the code. I do not have a preference, just wondering. 



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:678
+  if (StorageLocation *ExistingLoc =
+  getStorageLocation(RecordPRValue, SkipPast::None))
+return *cast(ExistingLoc);

Is eliminating `SkipPast` coming in a follow-up patch?



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:534-549
 void transferCallReturningOptional(const CallExpr *E,
const MatchFinder::MatchResult ,
LatticeTransferState ) {
   if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
 return;
 
+  AggregateStorageLocation *Loc = nullptr;

This is more of a note for the future of these APIs. I wonder if this is the 
right level of abstraction in this framework. I think it would be great to have 
something like:
```
void transferCallReturningOptional(const CallExpr *E,
   StorageLocation* RetLoc,
   ArrayRef Args,
   const MatchFinder::MatchResult ,
   LatticeTransferState );
```

and all the values and storage locations could come from the framework. It 
would be great if the user would almost never need to get a value or a location 
from an expression, they would be readily available. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155446

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

[PATCH] D155694: [NFC][analyzer] Enable implicit destructor for cfg-lifetime tests

[Gábor Horváth via Phabricator via cfe-commits]

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

Thanks!




Comment at: clang/test/Analysis/lifetime-cfg-output.cpp:6
+// that has non-trivial destructor. As the behavior for such types is different
+// from ones with trivial destructor - later ends they lifetime last, we
+// should extended the test cases. 

I have a bit hard time understanding this part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155694

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

[PATCH] D155694: [NFC][analyzer] Enable implicit destructor for cfg-lifetime tests

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D155694#4514322 , 
@tomasz-kaminski-sonarsource wrote:

> Updating this tests illustrated that we are missing a lot of coverage for the 
> objects with trivial destructors, but I think this should be addressed as a 
> separate patch.

Would it make sense to add some FIXME comments in this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155694

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

[PATCH] D155547: [analyzer] Model lifetime of a variable declared in for condition in CFG correctly

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/test/Analysis/lifetime-cfg-output.cpp:1
 // RUN: %clang_cc1 -fcxx-exceptions -fexceptions -analyze 
-analyzer-checker=debug.DumpCFG -analyzer-config 
cfg-lifetime=true,cfg-temporary-dtors=false,cfg-rich-constructors=false 
-analyzer-config cfg-implicit-dtors=false %s > %t 2>&1
 // RUN: FileCheck --input-file=%t %s

steakhal wrote:
> tomasz-kaminski-sonarsource wrote:
> > @xazax.hun 
> > A side question, this test disables `cfg-temporary-dtors`, 
> > `cfg-rich-constructors`, and `cfg-implicit-dtors` that are enabled by 
> > default for the analyzer. 
> > This was done because our changes [[ https://reviews.llvm.org/D153273 | 
> > D153273 ]] these functionalities were incompatible, and there were 
> > assertions triggering.
> > I would like to update the test to just enable `cfg-lifetime` similarly to 
> > what `cfg-scopes` do, so we catch any regressions, however, we will not 
> > test this functionality alone.
> > Is this something you (upstream) would accept? If so I can create NFC patch.
> Makes sense to me.
Sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155547

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

[PATCH] D155547: [analyzer] Model lifetime of a variable declared in for condition in CFG correctly

[Gábor Horváth via Phabricator via cfe-commits]

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

LG, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155547

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

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:122
+
+  using BlockOrderTy = llvm::DenseMap;
+  BlockOrderTy BlockOrder;

Would it make sense to have a vector instead and use block ids to get the order?



Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:101-103
+/// groups topologically. As a result, the blocks in a series of loops are
+/// ordered such that all nodes in loop `i` are earlier in the order than nodes
+/// in loop `j`. This ordering, when combined with widening, bounds the number

ymandel wrote:
> xazax.hun wrote:
> > I wonder if we still want to somehow enforce that within a loop/interval 
> > the we visit nodes in the RPO order.
> Starting with RPO should make this algorithm cheaper (and maybe simpler!).  
> But, RPO construction isn't free. So, I think it is a matter of cost -- 
> compare generating the RPO first and then using that here vs. just using this 
> directly.  I can put a note in the implementation as FIXME. WDYT?
Sounds good to me.



Comment at: clang/lib/Analysis/IntervalPartition.cpp:121
+Index.emplace(N, ID);
+  Graph.Intervals.emplace_back(ID, Header, std::move(Data.Nodes));
 }

ymandel wrote:
> xazax.hun wrote:
> > ymandel wrote:
> > > xazax.hun wrote:
> > > > It would probably take for me some time to understand what is going on 
> > > > here, but I think this part might worth a comment. In particular, I 
> > > > have the impression that `Graph.Intervals` is the owner of all the 
> > > > intervals. But we use pointers to refer to these intervals. What would 
> > > > guarantee that those pointers are not being invalidated by this 
> > > > `emplace_back` operations?
> > > Excellent question, not least because I *wasn't* careful the first around 
> > > and indeed ran up against this as a memory bug. :) That's the reason I 
> > > added IDs, so that I could _avoid_ taking the addresses of elements until 
> > > after we finish growing the vector. See lines 148-165: after we finish 
> > > building the intervals, we go through and take addresses.
> > > 
> > > I can add comments to this effect, but perhaps the safe option is to use 
> > > a std::dequeue. What do you think?
> > Oh, thanks! I don't have a strong preference between a comment or a deque. 
> I ended up changing to a deque after almost making the same mistake again. 
> vector is just not safe when you want to be taking addresses. :) I've dropped 
> the use of IDs and added comments. Let me know if this clear enough.
Looks good, thanks! I agree that the deque solution looks cleaner, probably we 
should get it first right and we can optimize later if there is a need for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058

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

[PATCH] D155547: [analyzer] Model lifetime of a variable declared in for condition in CFG correctly

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/test/Analysis/auto-obj-dtors-cfg-output.cpp:1231
   A a;
-  for (A b; A c = b; ) {
+  for (A b; A c = b; ++c.x) {
 A d;

Would there be any value in also keeping a couple of the original test cases? 
(When we have empty increment blocks). 

How about a test case where the increment operation generates multiple basic 
blocks (e.g., have a ternary)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155547

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

[PATCH] D155445: [analyzer][docs] Add CSA release notes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:908
+  (`7cd1f3ad22e4 `_)
+- Fixed a null-pointer dereference crash inside the ``MoveChecker``.
+  (`d172b65ef001 `_)

steakhal wrote:
> xazax.hun wrote:
> > steakhal wrote:
> > > xazax.hun wrote:
> > > > I think we usually do not mention crash fixes in the changelog. We have 
> > > > them in almost every release and sometimes there are quite a few of 
> > > > them.
> > > I won't mention the explicit commit where it was fixed.
> > > However, downstream users might wanna know about crashes and fixes that 
> > > happened in this release.
> > > And speaking about past practices about release notes, I think we can 
> > > improve on that TBH.
> > > We can move it down on the list if you want, but I'd rather keep it.
> > Is this the only crash fix we had? Moving crash fixes to the bottom of the 
> > list sounds good to me. 
> No, it wasn't. We also had one for init-expr global variable initializers. [[ 
> https://github.com/llvm/llvm-project/commit/558b46fde2db | See ]] 
> I swept that fix under the carpet of "Fixed some bugs around the handling of 
> constant global arrays and their initializer expressions". I made it more 
> explicit now.
> 
> However, at this point, I think it's okay to simply omit the mention of the 
> null deref crash fix.
> Second thoughts?
I have no strong preference, I am fine with both :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155445

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

[PATCH] D153273: [analyzer] Rework support for CFGScopeBegin, CFGScopeEnd, CFGLifetime elements

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

Thanks! Looks good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153273

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

[PATCH] D155445: [analyzer][docs] Add CSA release notes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:908
+  (`7cd1f3ad22e4 `_)
+- Fixed a null-pointer dereference crash inside the ``MoveChecker``.
+  (`d172b65ef001 `_)

steakhal wrote:
> xazax.hun wrote:
> > I think we usually do not mention crash fixes in the changelog. We have 
> > them in almost every release and sometimes there are quite a few of them.
> I won't mention the explicit commit where it was fixed.
> However, downstream users might wanna know about crashes and fixes that 
> happened in this release.
> And speaking about past practices about release notes, I think we can improve 
> on that TBH.
> We can move it down on the list if you want, but I'd rather keep it.
Is this the only crash fix we had? Moving crash fixes to the bottom of the list 
sounds good to me. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155445

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

[PATCH] D153273: [analyzer] Rework support for CFGScopeBegin, CFGScopeEnd, CFGLifetime elements

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/test/Analysis/lifetime-cfg-output.cpp:760
 // CHECK-NEXT:2: [B1.1]++
-// CHECK-NEXT:3: [B2.2] (Lifetime ends)
-// CHECK-NEXT:4: [B3.1] (Lifetime ends)
+// CHECK-NEXT:3: [B2.4] (Lifetime ends)
+// CHECK-NEXT:4: [B2.2] (Lifetime ends)

Would it be possible to add the variable name to the lifetime ends dumps? It 
would make interpreting these tests somewhat easier. (It is OK as a 
separate/follow-up patch.)



Comment at: clang/test/Analysis/nonreturn-destructors-cfg-output.cpp:99
+//
+// CHECK:   [B2 (NORETURN)]
+// CHECK-NEXT:1: [B3.3].~A() (Implicit destructor)

B3 and B5 look almost identical. Do we need both? If not, would there be a way 
to deduplicate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153273

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

[PATCH] D155445: [analyzer][docs] Add CSA release notes

[Gábor Horváth via Phabricator via cfe-commits]

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



Comment at: clang/docs/ReleaseNotes.rst:907
+  Any use of this flag will result in an error.
+  (`7cd1f3ad22e4 `_)
+- Fixed a null-pointer dereference crash inside the ``MoveChecker``.

I think we should mention  something like "Use -fstrict-flex-array= instead 
if necessary."



Comment at: clang/docs/ReleaseNotes.rst:908
+  (`7cd1f3ad22e4 `_)
+- Fixed a null-pointer dereference crash inside the ``MoveChecker``.
+  (`d172b65ef001 `_)

I think we usually do not mention crash fixes in the changelog. We have them in 
almost every release and sometimes there are quite a few of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155445

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

[PATCH] D155204: [clang][dataflow] Add `refreshStructValue()`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:706-711
+/// This function is primarily intended for use by checks that set custom
+/// properties on `StructValue`s to model the state of these values. Such 
checks
+/// should avoid modifying the properties of an existing `StructValue` because
+/// these changes would be visible to other `Environment`s that share the same
+/// `StructValue`. Instead, call `refreshStructValue()`, then set the 
properties
+/// on the new `StructValue` that it returns. Typical usage:

mboehme wrote:
> xazax.hun wrote:
> > I think this is fine for now, but I wonder if we should come up with an API 
> > where errors like this cannot happen. One potential way would be to no 
> > longer include these properties in the `StructValue`s, but have a separate 
> > mapping `StructValue => Properties`. So, one can update the mapping in an 
> > environment without any unintended consequences in other environments. 
> > I think this is fine for now, but I wonder if we should come up with an API 
> > where errors like this cannot happen.
> 
> Thanks for bringing this up.
> 
> I agree that this needs more improvement in the future. The underlying issue 
> is that `StructValue` isn't really a very useful concept.
> 
> `Value` works great for scalar values, where we do actually treat it as the 
> immutable value that it's intended to be. Attaching properties to values 
> makes a lot of sense in this context: If we're modelling a property of an 
> immutable value, then that property is presumably itself immutable.
> 
> However, the `Value` concept has never worked well for structs / records 
> because, when we mutate fields, we don't update the `StructValue`. We could 
> try to change this, but I don't think this would be worth it because 
> `StructValue` isn't a useful concept in C++ anyway. As the value categories 
> RFC discusses, prvalues of class type are a very niche concept in C++. The 
> only thing you can do with them is to initialize a result object -- you can't 
> otherwise perform any operations on them (i.e. access member variables or 
> call member functions). This has been part of the motivation for reducing 
> their importance as part of the value categories work.
> 
> I think we should continue down that path and, ultimately, maybe eliminate 
> `StructValue` entirely. So while introducing a mapping `StructValue => 
> Properties` would be a good option, I think an even better one would be to 
> start attaching properties to `AggregateStorageLocation`s instead of 
> `StructValue`s. Because `AggregateStorageLocation`s are in essence, mutable, 
> the mapping of `AggregateStorageLocation`s to property values would need to 
> be reflected in the `Environment` in some form. I think a natural way to do 
> this would be to model properties in a similar way to fields: An 
> `AggregateStorageLocation` would have a mapping `PropertyName => 
> StorageLocation`, and the values of the properties would then be tracked 
> through the existing `StorageLocation => Value` mapping in the environment.
> 
> Benefits:
> 
> - As already noted, this makes properties on `AggregateStorageLocation`s very 
> similar to fields, which is what they are typically used to model
> - If a particular analysis needs a storage location for a property, we 
> already have one. For example, UncheckedOptionalAccessModel.cpp currently 
> models the "value" property as a `PointerValue` so that it has a 
> `StorageLocation` that it can use as the return value of `optional::value()`. 
> Under the approach I'm proposing, the storage location would be available 
> from the framework, and the model for a specific analysis wouldn't have to do 
> anything extra.
> - We don't need any additional logic for joins / widening; the existing join 
> / widening logic for the `StorageLocation => Value` mapping would also handle 
> properties.
> 
> So this is where I think we can go in the future, and I agree that 
> `refreshStructValue()` should only be a stopgap as we evolve the framework.
This sounds great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155204

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

[PATCH] D155204: [clang][dataflow] Add `refreshStructValue()`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:706-711
+/// This function is primarily intended for use by checks that set custom
+/// properties on `StructValue`s to model the state of these values. Such 
checks
+/// should avoid modifying the properties of an existing `StructValue` because
+/// these changes would be visible to other `Environment`s that share the same
+/// `StructValue`. Instead, call `refreshStructValue()`, then set the 
properties
+/// on the new `StructValue` that it returns. Typical usage:

I think this is fine for now, but I wonder if we should come up with an API 
where errors like this cannot happen. One potential way would be to no longer 
include these properties in the `StructValue`s, but have a separate mapping 
`StructValue => Properties`. So, one can update the mapping in an environment 
without any unintended consequences in other environments. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155204

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

[PATCH] D154969: [dataflow] document flow condition

[Gábor Horváth via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:528
 
-  /// Returns the token that identifies the flow condition of the environment.
+  /// Returns a boolean variable that identifies the flow condition.
+  ///

The FC abbreviation is used later, so we should define it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154969

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

[PATCH] D154965: [clang][dataflow] Fix initializaing a reference field with an `InitListExpr`.

[Gábor Horváth via Phabricator via cfe-commits]

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



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:720-725
   for (auto It : llvm::zip(Fields, S->inits())) {
 const FieldDecl *Field = std::get<0>(It);
 assert(Field != nullptr);
 
 const Expr *Init = std::get<1>(It);
 assert(Init != nullptr);




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154965

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

[PATCH] D154935: [clang][dataflow] Introduce `getFieldValue()` test helpers.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: 
clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp:94
   Environment Env(DAContext, *Fun);
-  Value *Val = Env.createValue(Ty);
-  ASSERT_NE(Val, nullptr);
-  StructValue *SVal = clang::dyn_cast(Val);
-  ASSERT_NE(SVal, nullptr);
-  Val = SVal->getChild(*R);
-  ASSERT_NE(Val, nullptr);
-  PointerValue *PV = clang::dyn_cast(Val);
-  EXPECT_NE(PV, nullptr);
+  StructValue *SVal = clang::cast(Env.createValue(Ty));
+  PointerValue *PV = cast_or_null(getFieldValue(SVal, *R, Env));




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154935

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

[PATCH] D154478: [analyzer][NFC] Use unique_ptrs for PathDiagnosticConsumers

[Gábor Horváth via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154478

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

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:56-64
+  template  struct NodeData {
+// The block from which the interval was constructed. It is either the
+// graph's entry block or has at least one predecessor outside the 
interval.
+const Node *Header;
+std::vector Nodes;
+  };
+  using IntervalData = std::variant, NodeData>;

Correct me if I'm wrong but I have the impression that `CFGInterval` might 
either contain only `CfgBlock`s or other `CfgInterval`s, but these might never 
be mixed. The current representation does allow such mixing. In case do not 
need that, I think it might be cleaner to make `CfgInterval` itself templated 
and remove the `std::variant`.



Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:101-103
+/// groups topologically. As a result, the blocks in a series of loops are
+/// ordered such that all nodes in loop `i` are earlier in the order than nodes
+/// in loop `j`. This ordering, when combined with widening, bounds the number

I wonder if we still want to somehow enforce that within a loop/interval the we 
visit nodes in the RPO order.



Comment at: clang/lib/Analysis/IntervalPartition.cpp:36
+bool inInterval(const Node *N, std::vector ) {
+  return std::find(Interval.begin(), Interval.end(), N) != Interval.end();
+}

We have some convenience wrappers in LLVM: 
https://llvm.org/doxygen/namespacellvm.html#a086e9fbdb06276db7753101a08a63adf



Comment at: clang/lib/Analysis/IntervalPartition.cpp:121
+Index.emplace(N, ID);
+  Graph.Intervals.emplace_back(ID, Header, std::move(Data.Nodes));
 }

ymandel wrote:
> xazax.hun wrote:
> > It would probably take for me some time to understand what is going on 
> > here, but I think this part might worth a comment. In particular, I have 
> > the impression that `Graph.Intervals` is the owner of all the intervals. 
> > But we use pointers to refer to these intervals. What would guarantee that 
> > those pointers are not being invalidated by this `emplace_back` operations?
> Excellent question, not least because I *wasn't* careful the first around and 
> indeed ran up against this as a memory bug. :) That's the reason I added IDs, 
> so that I could _avoid_ taking the addresses of elements until after we 
> finish growing the vector. See lines 148-165: after we finish building the 
> intervals, we go through and take addresses.
> 
> I can add comments to this effect, but perhaps the safe option is to use a 
> std::dequeue. What do you think?
Oh, thanks! I don't have a strong preference between a comment or a deque. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058

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

[PATCH] D151325: [analyzer] Differentiate lifetime extended temporaries

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:556
+  bool IsLocal =
+  isa_and_nonnull(MR) &&
+  isa(MR->getMemorySpace());

tomasz-kaminski-sonarsource wrote:
> xazax.hun wrote:
> > I think strictly speaking this might be "unsound" resulting in false 
> > positives in scenarios like:
> > ```
> > void f(bool reset) {
> >   static T&& extended = getTemp();
> >   if (reset) {
> > extended = getTemp();
> > return;
> >   }
> >   consume(std::move(extended));
> >   f(true);
> >   extended.use();
> > }
> > ```
> > 
> > In case the call to `f(true)` is not inlined (or in other cases there is 
> > some aliasing the analyzer does not know about), we might not realize that 
> > the object is reset and might falsely report a use after move.
> > 
> > But I think this is probably sufficiently rare that we do not care about 
> > those. 
> Aren't such case already excluded by the 
> `isa(MR->getMemorySpace())`, in a same way as we do for 
> variables?
> In such case, we should be creating 
> `getCXXStaticLifetimeExtendedObjectRegion` so it will not be located on stack.
Whoops, indeed, sorry. 



Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:101
+QualType Ty = LER->getValueType().getLocalUnqualifiedType();
+os << "stack memory associated with temporary object of type '";
+Ty.print(os, Ctx.getPrintingPolicy());

tomasz-kaminski-sonarsource wrote:
> xazax.hun wrote:
> > Is this a good wording for static lifetime extended temporaries?
> Lifetime extended static temporaries should never enter here, as they are not 
> in `StackSpaceRegion`. 
> Previously we have had `CXXStaticTempObjects` and now they are turned into 
> `CXXStaticLifetimeExtendedObjectRegion`.
Ah, my bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151325

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

[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

I love it, I think we can land this as is. If there are further comments, we 
can address those in follow-up PRs.




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1190
   CXXRecordDecl::field_iterator CurField = LE->getLambdaClass()->field_begin();
-  for (LambdaExpr::const_capture_init_iterator i = LE->capture_init_begin(),
-   e = LE->capture_init_end();
-   i != e; ++i, ++CurField, ++Idx) {
+  for (auto i = LE->capture_init_begin(), e = LE->capture_init_end(); i != e;
+   ++i, ++CurField, ++Idx) {

steakhal wrote:
> steakhal wrote:
> > xazax.hun wrote:
> > > Ha about using `capture_inits`?
> > I would count this as a "fancy" iteration as we increment/advance more 
> > logical sequences together.
> I had a better idea and llvm::zip all these together.
> Check it out, and tell me if you like it more ;)
> I certainly do.
Nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154325

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

[PATCH] D151325: [analyzer] Differentiate lifetime extended temporaries

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:556
+  bool IsLocal =
+  isa_and_nonnull(MR) &&
+  isa(MR->getMemorySpace());

I think strictly speaking this might be "unsound" resulting in false positives 
in scenarios like:
```
void f(bool reset) {
  static T&& extended = getTemp();
  if (reset) {
extended = getTemp();
return;
  }
  consume(std::move(extended));
  f(true);
  extended.use();
}
```

In case the call to `f(true)` is not inlined (or in other cases there is some 
aliasing the analyzer does not know about), we might not realize that the 
object is reset and might falsely report a use after move.

But I think this is probably sufficiently rare that we do not care about those. 



Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:101
+QualType Ty = LER->getValueType().getLocalUnqualifiedType();
+os << "stack memory associated with temporary object of type '";
+Ty.print(os, Ctx.getPrintingPolicy());

Is this a good wording for static lifetime extended temporaries?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151325

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

[PATCH] D151325: [analyzer] Differentiate lifetime extended temporaries

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1282
+  LLVM_ATTRIBUTE_RETURNS_NONNULL
+  const Expr *getExpr() const { return Ex; }
+  LLVM_ATTRIBUTE_RETURNS_NONNULL

tomasz-kaminski-sonarsource wrote:
> xazax.hun wrote:
> > Do we actually need this `Expr` in the memory region?
> Yes they are needed for region identification, as one variable my be lifetime 
> extending multiple temporary.
> For illustration consider following example from test:
> ```
>   auto const& viaReference = RefAggregate{10, Composite{}}; // extends `int`, 
> `Composite`, and `RefAggregate`
>   clang_analyzer_dump(viaReference);// expected-warning-re 
> {{_extended_object{RefAggregate, viaReference, S{{[0-9]+}}} }}
>   clang_analyzer_dump(viaReference.rx); // expected-warning-re 
> {{_extended_object{int, viaReference, S{{[0-9]+}}} }}
>   clang_analyzer_dump(viaReference.ry); // expected-warning-re 
> {{_extended_object{Composite, viaReference, S{{[0-9]+}}} }}
> ```
> `viaReference` declaration is extending 3 temporaries, and they get 
> identified by expression.
Ah, OK. Makes sense, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151325

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

[PATCH] D151325: [analyzer] Differentiate lifetime extended temporaries

[Gábor Horváth via Phabricator via cfe-commits]

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

Overall looks good to me. I think the changes to the notes already make the 
analyzer more useful so there are some observable benefits to this patch.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1282
+  LLVM_ATTRIBUTE_RETURNS_NONNULL
+  const Expr *getExpr() const { return Ex; }
+  LLVM_ATTRIBUTE_RETURNS_NONNULL

Do we actually need this `Expr` in the memory region?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151325

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

[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:501
   public:
+const iterator *() const { return *this; }
+

Is this just a dummy to make sure this satisfies some concepts? I think I 
comment might be helpful in that case, people might find a noop dereference 
operator confusing. Same for some other places.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:749-750
 
   referenced_vars_iterator referenced_vars_begin() const;
   referenced_vars_iterator referenced_vars_end() const;
+  llvm::iterator_range referenced_vars() const;

Do we need these with the addition of `referenced_vars`?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:637-638
 
   region_iterator region_begin() const { return LiveRegionRoots.begin(); }
   region_iterator region_end() const { return LiveRegionRoots.end(); }
+  llvm::iterator_range regions() const {

Should we remove these?



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp:194-195
 
-  for (CallExpr::const_arg_iterator ai = i->AllocCall->arg_begin(),
-   ae = i->AllocCall->arg_end(); ai != ae; ++ai) {
-if (!(*ai)->getType()->isIntegralOrUnscopedEnumerationType())
+  for (const Expr *Arg : make_range(CallRec.AllocCall->arg_begin(),
+CallRec.AllocCall->arg_end())) {
+if (!Arg->getType()->isIntegralOrUnscopedEnumerationType())





Comment at: clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp:54
+  for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+delete Consumer;
   }

Hm, I wonder whether we actually want to use `unique_ptr`s to make the 
ownership explicit. Feel free to leave as is in this PR.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:173-177
+} else {
 // The left-hand side may bind to a different value then the
 // computation type.
 LHSVal = svalBuilder.evalCast(Result, LTy, CTy);
+}

Might be an issue with Phab, but indent looks strange to me here.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1190
   CXXRecordDecl::field_iterator CurField = LE->getLambdaClass()->field_begin();
-  for (LambdaExpr::const_capture_init_iterator i = LE->capture_init_begin(),
-   e = LE->capture_init_end();
-   i != e; ++i, ++CurField, ++Idx) {
+  for (auto i = LE->capture_init_begin(), e = LE->capture_init_end(); i != e;
+   ++i, ++CurField, ++Idx) {

Ha about using `capture_inits`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154325

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

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Analysis/IntervalPartition.cpp:121
+Index.emplace(N, ID);
+  Graph.Intervals.emplace_back(ID, Header, std::move(Data.Nodes));
 }

It would probably take for me some time to understand what is going on here, 
but I think this part might worth a comment. In particular, I have the 
impression that `Graph.Intervals` is the owner of all the intervals. But we use 
pointers to refer to these intervals. What would guarantee that those pointers 
are not being invalidated by this `emplace_back` operations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058

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

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/unittests/Analysis/IntervalPartitionTest.cpp:21
+namespace {
+template  using NodeData = CFGInterval::NodeData;
+}  // namespace

Another redundant anonymous namespace here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058

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

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Could you rebase this patch to the latest tip of tree?




Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:73
+
+  // Whether this node is the head of a feedback edge within the interval.
+  bool IsFeedbackHead = false;

Is feedback edge a special terminology? I think we might simply call these back 
edges most of the time.



Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:83
 
-// Partitions `Cfg` into intervals and constructs a graph of the intervals,
+// Partitions `Cfg` into intervals and constructs a the graph of the intervals
 // based on the edges between nodes in these intervals.

typo?



Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:87
+
+// (Further) partitions `Graph` into intervals and constructs a the graph of 
the
+// intervals based on the edges between nodes (themselves intervals) in these

same typo here?



Comment at: clang/lib/Analysis/IntervalPartition.cpp:23-25
+namespace {
+template  using NodeData = CFGInterval::NodeData;
+} // namespace

What is the reason for putting a using statement in an anonymous namespace? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058

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

[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152263

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

[PATCH] D153674: [dataflow] Disallow implicit copy of Environment, use fork() instead

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

Nice, looks like this change did catch some unintentional copies! Already 
paying dividends :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153674

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

[PATCH] D153491: [dataflow] Avoid copying environment

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D153491#4445051 , @ymandel wrote:

> In D153491#4443704 , @xazax.hun 
> wrote:
>
>> This sounds extremely error-prone to me. In case copying the analysis state 
>> has side effects like this, I would argue we want such operations to be 
>> really explicit. What do you think?
>
> Can you expand on this concern? Are you referring to the removal of the 
> unintended copy (ie. this patch), or raising a concern about the how the 
> underlying system handles copies altogether?

The latter, https://reviews.llvm.org/D153674 should address my concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153491

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

[PATCH] D153584: [dataflow] Make SAT solver deterministic

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Is there a measurable perf cost for this determinism?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153584

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

[PATCH] D153488: [dataflow] HTMLLogger: meaningful names for flow condition variables

[Gábor Horváth via Phabricator via cfe-commits]

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

Should we have some documentation or tooltip explaining the users what the 
meaning of those names are?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153488

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

[PATCH] D153476: [dataflow] document & test determinism of formula structure

[Gábor Horváth via Phabricator via cfe-commits]

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



Comment at: clang/lib/Analysis/FlowSensitive/Arena.cpp:13
 
+// Compute a canonical key for an unordered poir of operands, so that we
+// can intern (A & B) and (B & A) to the same formula.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153476

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

[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

[Gábor Horváth via Phabricator via cfe-commits]

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



Comment at: clang/lib/Analysis/IntervalPartition.cpp:26
+
+  std::queue Worklist;
+  for (const CFGBlock *S : Header.succs())

ymandel wrote:
> xazax.hun wrote:
> > Is it possible we end up adding the same node to the queue multiple times? 
> > Is that desirable or do we want to make sure we only have each node at most 
> > once?
> Added an answer in comments, but repeating here in case you want to discuss 
> further:
> 
>   // The worklist *may* contain duplicates. We guard against this possibility 
> by
>   // checking each popped element for completion (that is, presence in
>   // `Partitioned`). We choose this approach over using a set because the 
> queue
>   // allows us to flexibly insert and delete elements while iterating through
>   // the list. A set would require two separate phases for iterating and
>   // mutation.
I see, thanks! This addresses my concerns. I think in some cases we use a 
bitset with the blocks' ids to more efficiently track things like that.



Comment at: clang/lib/Analysis/IntervalPartition.cpp:46-47
+
+// Check whether all predecessors are in the interval, in which case `B`
+// is included as well.
+bool AllInInterval = true;

ymandel wrote:
> xazax.hun wrote:
> > I wonder if this approach is correct. Consider the following scenario:
> > 
> > ```
> >A
> >   / \
> >  B   C
> >  |   |
> >  |   D
> >   \ /
> >E
> > ```
> > 
> > In the BFS, we might visit: ABCED. Since we visit `E` before `D`, we might 
> > not recognize that `E` is part of the interval. Do I miss something?
> > I wonder if this approach is correct. Consider the following scenario:
> > 
> > ```
> >A
> >   / \
> >  B   C
> >  |   |
> >  |   D
> >   \ /
> >E
> > ```
> > 
> > In the BFS, we might visit: ABCED. Since we visit `E` before `D`, we might 
> > not recognize that `E` is part of the interval. Do I miss something?
> 
> When we add `D` to the interval, we'll push `E` onto the queue again (lines 
> 58-59). The second time that `E` is considered it will have both successors 
> in the interval and will be added as well.
Ah, I see, makse sense, thanks! This also makes me wonder, would this algorithm 
be more efficient if we used RPO (in case we can use that without recalculating 
the order)? :D But we do not need to benchmark this for the first version. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152263

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

[PATCH] D153491: [dataflow] Avoid copying environment

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

This sounds extremely error-prone to me. In case copying the analysis state has 
side effects like this, I would argue we want such operations to be really 
explicit. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153491

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

[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:35
+
+  std::set Blocks;
+

Nit: I wonder if we want something like `llvm::DenseSet` when we use smaller 
types like pointers. Same for `Successors`.



Comment at: clang/lib/Analysis/IntervalPartition.cpp:26
+
+  std::queue Worklist;
+  for (const CFGBlock *S : Header.succs())

Is it possible we end up adding the same node to the queue multiple times? Is 
that desirable or do we want to make sure we only have each node at most once?



Comment at: clang/lib/Analysis/IntervalPartition.cpp:37
+  // successors.
+  std::vector MaybeSuccessors;
+

Same question here, is it possible we might end up adding the same nodes 
multiple times? 



Comment at: clang/lib/Analysis/IntervalPartition.cpp:46-47
+
+// Check whether all predecessors are in the interval, in which case `B`
+// is included as well.
+bool AllInInterval = true;

I wonder if this approach is correct. Consider the following scenario:

```
   A
  / \
 B   C
 |   |
 |   D
  \ /
   E
```

In the BFS, we might visit: ABCED. Since we visit `E` before `D`, we might not 
recognize that `E` is part of the interval. Do I miss something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152263

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

[PATCH] D153006: [clang][dataflow] Perform deep copies in copy and move operations.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/RecordOps.cpp:56-57
+
+  llvm::iterator_range<
+  llvm::DenseMap::const_iterator>
+  DstChildren = DstVal->children();

I know we usually like to mention types at least once, but wonder whether 
ranges/iterators are actually exceptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153006

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

[PATCH] D153006: [clang][dataflow] Perform deep copies in copy and move operations.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I am not opposed to this solution, but I do anticipate some performance 
problems in the future. I wonder if copy-on-write, or some persistent data 
structures where copying is cheap would be a better strategy long term. But 
getting it right first and fast after sounds like a good strategy :)




Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:754-755
+
+  auto It = MemberLocToStruct.find();
+  if (It != MemberLocToStruct.end()) {
+// `Loc` is the location of a struct member so we need to also clear the




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153006

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

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Arena.h:89
+  /// `Value`. These literals are the same every time.
+  IntegerValue (llvm::APInt Value);
+

gribozavr2 wrote:
> Should we be taking the type into account? If not, why not? Please add the 
> type or document why the type isn't tracked.
What would happen if we try to create the same value (like 5) but with 
different bit widths? (E.g., 64 bit integer vs 32 bit integer). I suspect we 
might end up having the same value twice in our constant pool, which might be 
fine. Just wanted to double check what is the desired behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152813

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

[PATCH] D152732: [clang][dataflow] Support limits on the SAT solver to force timeouts.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D152732#4414707 , @ymandel wrote:

> Ultimately what matters for a user is the global limit.

I am not 100% sure about that. While it is true that the user cares about the 
process not hanging, but global vs local limits can have observable effects on 
the analysis results. With a global limit, after a query exhausted all the 
budget, for all intents and purposes we continue the analysis without a solver 
for the rest of the function and all queries would just time out, even the 
simple ones. With a local limit, the solver might time out for a couple of 
queries, but we keep the precision for the simple queries. That being said, it 
is possible that the scenario where we have a few big queries that blows the 
solver up but the rest of them are simple just does not happen that much. Also, 
a local timeout produces less reliable worst case runtime results. This makes 
me think it might be possible that we want both, but this decision is probably 
better made when we have some evidence that we actually need both. So, I am ok 
with committing this as is for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152732

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

[PATCH] D152732: [clang][dataflow] Support limits on the SAT solver to force timeouts.

[Gábor Horváth via Phabricator via cfe-commits]

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

Huge +1, I think most solvers need to have some resource limits in place as the 
runtime can explode. I am just not 100% what is the best approach here, putting 
a global limit on the solver instance vs having a limit per query. Any thoughts 
on that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152732

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

[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:22
+
+struct CFGInterval {
+  CFGInterval(const CFGBlock *Header) : Header(Header), Blocks({Header}) {}

A concise definition of what is an interval with a reference to the dragon book 
might be useful here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152263

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

[PATCH] D151818: [clang][analyzer][NFC] Use the operator new directly with the `BumpPtrAllocator`

[Gábor Horváth via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151818

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

[PATCH] D151194: [clang][dataflow] Add support for return values of reference type.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:196
+  void popCall(const CallExpr *Call, const Environment );
+  void popCall(const CXXConstructExpr *Call, const Environment );
 

mboehme wrote:
> xazax.hun wrote:
> > I know that Obj-C is a non-goal, but it might worth a comment to support 
> > `ObjCMessageExpr` just in case someone wants to work on this. 
> > 
> > Btw, this is one of my biggest pet peeves about the Clang AST. We should 
> > have a common abstraction for all the callables, instead of having to 
> > bifurcate many of the APIs. 
> Agreed!
> 
> I'm a bit hesitant to add a FIXME here though, as that might make it sound as 
> if there's a plan to add Objective-C support or at least imply that that's a 
> goal. Also, I think Objective-C support would entail quite a bit more than 
> adding the right overloads here -- so I'm not sure how helpful the comment 
> here would be. I think once someone goes down the road of seriously working 
> on Objective-C support, they'll quickly discover that they need a new 
> overload here.
> 
> I hope this makes sense? Happy to add something in a followup patch if you 
> feel we should add some Objective-C "breadcrumbs" here.
Yup, makes sense!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151194

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

[PATCH] D151325: [analyzer] Differentiate lifetime extended temporaries

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I generally support the idea of a CXXLifetimeExtendedObj region, definitely 
cleaner than a CXXTempObjectRegion with static lifetime.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151325

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

[PATCH] D151194: [clang][dataflow] Add support for return values of reference type.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:196
+  void popCall(const CallExpr *Call, const Environment );
+  void popCall(const CXXConstructExpr *Call, const Environment );
 

I know that Obj-C is a non-goal, but it might worth a comment to support 
`ObjCMessageExpr` just in case someone wants to work on this. 

Btw, this is one of my biggest pet peeves about the Clang AST. We should have a 
common abstraction for all the callables, instead of having to bifurcate many 
of the APIs. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151194

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

[PATCH] D151201: [clang][dataflow] Fix a crash in `getLogicOperatorSubExprValue()`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:5064
+bool target() {
+  return true || false || false || false;
+}

Should we also test that the value of the expression is `true` in the analysis 
state?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151201

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

[PATCH] D151183: [clang][dataflow] Add a `ControlFlowContext::build()` overload taking a `FunctionDecl`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.



Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:39
+
   /// Builds a ControlFlowContext from an AST node. `D` is the function in 
which
   /// `S` resides. `D.isTemplated()` must be false.

I was wondering if there is a plan to make the framework work for 
non-functions, like global initializers. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151183

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

[PATCH] D150655: [clang][dataflow] Use `Strict` accessors in more places in Transfer.cpp.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:717-723
+Value *SubExprVal = Env.getValueStrict(*SubExpr);
+if (SubExprVal == nullptr)
   return;
 
-Env.setStorageLocation(*S, *SubExprLoc);
+auto  = Env.createStorageLocation(*S);
+Env.setStorageLocationStrict(*S, Loc);
+Env.setValue(Loc, *SubExprVal);

This operation is basically something like an RValue to LValue cast. I am not 
sure how many of these will we have, but it might be a good idea to look out if 
we want to create something like `propagateAsLValue` similar to 
`propagateValue`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150655

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

[PATCH] D150657: [clang][dataflow] Use `Strict` accessors in SignAnalysisTest.cpp.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:134
 LatticeTransferState ) {
-  StorageLocation *Loc = State.Env.getStorageLocation(*BO, SkipPast::None);
-  if (!Loc) {
-Loc = (*BO);
-State.Env.setStorageLocation(*BO, *Loc);
-  }
-  BoolValue *Comp = cast_or_null(State.Env.getValue(*Loc));
+  BoolValue *Comp = cast_or_null(State.Env.getValueStrict(*BO));
   if (!Comp) {

Wow, this is so much cleaner than before!



Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:240
 
+// Returns the `Value` associated with `E` (which may be either a prvalue or
+// glvalue). Creates a `Value` or `StorageLocation` as needed if `E` does not

sammccall wrote:
> mboehme wrote:
> > sammccall wrote:
> > > this seems to belong on Environment rather than here!
> > I've consciously kept this function local to this file for now because it 
> > has relatively complex semantics and I haven't seen any other places where 
> > these are required so far. I've added a comment noting that this should be 
> > moved if it turns out to be needed elsewhere.
> > 
> > I think that, as much as possible, the methods on `Environment` should be 
> > specific about the value category they operate on, because most of the 
> > time, it's known what the value category is, and I'd like to encourage 
> > users to use the most specific operation possible (because using more 
> > general operations breeds bugs).
> > I think that, as much as possible, the methods on Environment should be 
> > specific about the value category they operate on
> 
> I agree, but don't think that's specific to Environment.
> I think my favorite factoring of this would be to inline the if/else and move 
> the branches to methods on environment
> 
> ```
> Value *Val = E->isGLValue() ? Env.getOrCreateLValue(*E) : 
> Env.getOrCreateValue(*E);
> ```
> 
> but this is test code, it doesn't matter much, up to you.
So, we could either make an API that forces users to think about value 
categories explicitly, or we could try to design an API where the users do not 
need to know about value categories at all. I think it might be worth exploring 
if the latter is possible without any major caveats, the simpler the APIs are 
the better. But this is for the future, not for this PR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150657

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

[PATCH] D150656: [clang][dataflow] Use `Strict` accessors in TypeErasedDataflowAnalysis.cpp.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:303
 
-  auto *InitStmtLoc = Env.getStorageLocation(*InitStmt, SkipPast::Reference);
-  if (InitStmtLoc == nullptr)
-return;
-
-  auto *InitStmtVal = Env.getValue(*InitStmtLoc);
-  if (InitStmtVal == nullptr)
-return;
-
   if (Member->getType()->isReferenceType()) {
+auto *InitStmtLoc = Env.getStorageLocationStrict(*InitStmt);

I am wondering whether it is more robust to branch directly on the value 
category of `InitStmt`, although I cannot think of cases where this condition 
would go wrong. Feel free to ignore. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150656

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

[PATCH] D149144: [clang][dataflow] Eliminate intermediate `ReferenceValue`s from `Environment::DeclToLoc`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329
+static void
+builtinTransferScopeEnd(const CFGScopeEnd ,
+TypeErasedDataflowAnalysisState ) {

mboehme wrote:
> xazax.hun wrote:
> > mboehme wrote:
> > > mboehme wrote:
> > > > xazax.hun wrote:
> > > > > I think one question is whether we are interested in ScopeEnd or 
> > > > > LifetimeEnd, see the discussions in https://reviews.llvm.org/D15031 
> > > > > and https://reviews.llvm.org/D16403, specifically Devin's comment: 
> > > > > 
> > > > > >>! In D16403#799926, @dcoughlin wrote:
> > > > > >> @dcoughlin As a reviewer of both patches - could you tell us 
> > > > > >> what's the difference between them? And how are we going to 
> > > > > >> resolve this issue?
> > > > > > 
> > > > > > These two patches are emitting markers in the CFG for different 
> > > > > > things.
> > > > > > 
> > > > > > Here is how I would characterize the difference between the two 
> > > > > > patches.
> > > > > > 
> > > > > > - Despite its name, https://reviews.llvm.org/D15031, is really 
> > > > > > emitting markers for when the lifetime of a C++ object in an 
> > > > > > automatic variable ends.  For C++ objects with non-trivial 
> > > > > > destructors, this point is when the destructor is called. At this 
> > > > > > point the storage for the variable still exists, but what you can 
> > > > > > do with that storage is very restricted by the language because its 
> > > > > > contents have been destroyed. Note that even with the contents of 
> > > > > > the object have been destroyed, it is still meaningful to, say, 
> > > > > > take the address of the variable and construct a new object into it 
> > > > > > with a placement new. In other words, the same variable can have 
> > > > > > different objects, with different lifetimes, in it at different 
> > > > > > program points.
> > > > > > 
> > > > > > - In contrast, the purpose of this patch 
> > > > > > (https://reviews.llvm.org/D16403) is to add markers for when the 
> > > > > > storage duration for the variable begins and ends (this is, when 
> > > > > > the storage exists). Once the storage duration has ended, you can't 
> > > > > > placement new into the variables address, because another variable 
> > > > > > may already be at that address.
> > > > > > 
> > > > > > I don't think there is an "issue" to resolve here.  We should make 
> > > > > > sure the two patches play nicely with each other, though. In 
> > > > > > particular, we should make sure that the markers for when lifetime 
> > > > > > ends and when storage duration ends are ordered correctly.
> > > > > 
> > > > > 
> > > > > What I wanted to add, I wonder if we might not get ScopeEnd event for 
> > > > > temporaries and there might be other differences. The Clang 
> > > > > implementation of P1179 used LifetimeEnd instead of ScopeEnd, and I 
> > > > > believe probably most clients are more interested in LifetimeEnd 
> > > > > events rather than ScopeEnd.
> > > > > 
> > > > > I think I understand why you went with ScopeEnd for this specific 
> > > > > problem, but to avoid the confusion from having both in the Cfg 
> > > > > (because I anticipate someone will need LifetimeEnd at some point), I 
> > > > > wonder if we can make this work with LifetimeEnd. 
> > > > Hm, thanks for bringing it up.
> > > > 
> > > > Conincidentally, I realized while chasing another issue that 
> > > > `CFGScopeEnd` isn't the right construct here. I assumed that we would 
> > > > get a `CFGScopeEnd` for every variable, but I now realize that we only 
> > > > get a `CFGScopeEnd` for the first variable in the scope.
> > > > 
> > > > So `CFGLifetimeEnds` definitely looks like the right construct to use 
> > > > here, and indeed it's what I originally tried to use. Unfortuately, 
> > > > `CFG::BuildOptions::AddLifetime` cannot be used at the same time as 
> > > > `AddImplicitDtors`, which we already use. We don't actually need the 
> > > > `CFGElement`s added by `AddImplicitDtors`, but we do need 
> > > > `AddTemporaryDtors`, and that in turn can only be turned on if 
> > > > `AddImplicitDtors` is turned on.
> > > > 
> > > > It looks as if I will need to break one of these constraints. It looks 
> > > > as if the constraint that is easiest to break is that `AddLifetime` 
> > > > currently cannot be used at the same time as `AddImplicitDtors`. I'm 
> > > > not sure why this constraint currently exists (I'd appreciate any 
> > > > insights you or others may have), but I suspect it's because it's hard 
> > > > to ensure that the `CFGElement`s added by `AddImplicitDtors` are 
> > > > ordered correctly with respect to the `CFGLifetimeEnds` elements.
> > > > 
> > > > Anyway, this requires a change to `CFG` one way or another. I'll work 
> > > > on that next. I'll then make the required changes to this patch and 
> > > > will ask for another look before submitting.
> > > I've 

[PATCH] D149144: [clang][dataflow] Eliminate intermediate `ReferenceValue`s from `Environment::DeclToLoc`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329
+static void
+builtinTransferScopeEnd(const CFGScopeEnd ,
+TypeErasedDataflowAnalysisState ) {

mboehme wrote:
> mboehme wrote:
> > xazax.hun wrote:
> > > I think one question is whether we are interested in ScopeEnd or 
> > > LifetimeEnd, see the discussions in https://reviews.llvm.org/D15031 and 
> > > https://reviews.llvm.org/D16403, specifically Devin's comment: 
> > > 
> > > >>! In D16403#799926, @dcoughlin wrote:
> > > >> @dcoughlin As a reviewer of both patches - could you tell us what's 
> > > >> the difference between them? And how are we going to resolve this 
> > > >> issue?
> > > > 
> > > > These two patches are emitting markers in the CFG for different things.
> > > > 
> > > > Here is how I would characterize the difference between the two patches.
> > > > 
> > > > - Despite its name, https://reviews.llvm.org/D15031, is really emitting 
> > > > markers for when the lifetime of a C++ object in an automatic variable 
> > > > ends.  For C++ objects with non-trivial destructors, this point is when 
> > > > the destructor is called. At this point the storage for the variable 
> > > > still exists, but what you can do with that storage is very restricted 
> > > > by the language because its contents have been destroyed. Note that 
> > > > even with the contents of the object have been destroyed, it is still 
> > > > meaningful to, say, take the address of the variable and construct a 
> > > > new object into it with a placement new. In other words, the same 
> > > > variable can have different objects, with different lifetimes, in it at 
> > > > different program points.
> > > > 
> > > > - In contrast, the purpose of this patch 
> > > > (https://reviews.llvm.org/D16403) is to add markers for when the 
> > > > storage duration for the variable begins and ends (this is, when the 
> > > > storage exists). Once the storage duration has ended, you can't 
> > > > placement new into the variables address, because another variable may 
> > > > already be at that address.
> > > > 
> > > > I don't think there is an "issue" to resolve here.  We should make sure 
> > > > the two patches play nicely with each other, though. In particular, we 
> > > > should make sure that the markers for when lifetime ends and when 
> > > > storage duration ends are ordered correctly.
> > > 
> > > 
> > > What I wanted to add, I wonder if we might not get ScopeEnd event for 
> > > temporaries and there might be other differences. The Clang 
> > > implementation of P1179 used LifetimeEnd instead of ScopeEnd, and I 
> > > believe probably most clients are more interested in LifetimeEnd events 
> > > rather than ScopeEnd.
> > > 
> > > I think I understand why you went with ScopeEnd for this specific 
> > > problem, but to avoid the confusion from having both in the Cfg (because 
> > > I anticipate someone will need LifetimeEnd at some point), I wonder if we 
> > > can make this work with LifetimeEnd. 
> > Hm, thanks for bringing it up.
> > 
> > Conincidentally, I realized while chasing another issue that `CFGScopeEnd` 
> > isn't the right construct here. I assumed that we would get a `CFGScopeEnd` 
> > for every variable, but I now realize that we only get a `CFGScopeEnd` for 
> > the first variable in the scope.
> > 
> > So `CFGLifetimeEnds` definitely looks like the right construct to use here, 
> > and indeed it's what I originally tried to use. Unfortuately, 
> > `CFG::BuildOptions::AddLifetime` cannot be used at the same time as 
> > `AddImplicitDtors`, which we already use. We don't actually need the 
> > `CFGElement`s added by `AddImplicitDtors`, but we do need 
> > `AddTemporaryDtors`, and that in turn can only be turned on if 
> > `AddImplicitDtors` is turned on.
> > 
> > It looks as if I will need to break one of these constraints. It looks as 
> > if the constraint that is easiest to break is that `AddLifetime` currently 
> > cannot be used at the same time as `AddImplicitDtors`. I'm not sure why 
> > this constraint currently exists (I'd appreciate any insights you or others 
> > may have), but I suspect it's because it's hard to ensure that the 
> > `CFGElement`s added by `AddImplicitDtors` are ordered correctly with 
> > respect to the `CFGLifetimeEnds` elements.
> > 
> > Anyway, this requires a change to `CFG` one way or another. I'll work on 
> > that next. I'll then make the required changes to this patch and will ask 
> > for another look before submitting.
> I've taken a look at what it would take to make `AddLifetime` coexist with 
> `AddImplicitDtors`, and it's hard (because we need to get the ordering right, 
> and that's non-trivial).
> 
> So I've instead decided to remove the behavior from this patch that removes 
> declarations from `Environment::DeclToLoc` when they go out of scope and have 
> instead 

[PATCH] D149144: [clang][dataflow] Eliminate intermediate `ReferenceValue`s from `Environment::DeclToLoc`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a subscriber: dcoughlin.
xazax.hun added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:616
+  bool erased = DeclToLoc.erase();
+  if (!erased)
+D.dump();

Would we dump this in release builds? Do we want to limit this to debug/assert 
builds?



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329
+static void
+builtinTransferScopeEnd(const CFGScopeEnd ,
+TypeErasedDataflowAnalysisState ) {

I think one question is whether we are interested in ScopeEnd or LifetimeEnd, 
see the discussions in https://reviews.llvm.org/D15031 and 
https://reviews.llvm.org/D16403, specifically Devin's comment: 

>>! In D16403#799926, @dcoughlin wrote:
>> @dcoughlin As a reviewer of both patches - could you tell us what's the 
>> difference between them? And how are we going to resolve this issue?
> 
> These two patches are emitting markers in the CFG for different things.
> 
> Here is how I would characterize the difference between the two patches.
> 
> - Despite its name, https://reviews.llvm.org/D15031, is really emitting 
> markers for when the lifetime of a C++ object in an automatic variable ends.  
> For C++ objects with non-trivial destructors, this point is when the 
> destructor is called. At this point the storage for the variable still 
> exists, but what you can do with that storage is very restricted by the 
> language because its contents have been destroyed. Note that even with the 
> contents of the object have been destroyed, it is still meaningful to, say, 
> take the address of the variable and construct a new object into it with a 
> placement new. In other words, the same variable can have different objects, 
> with different lifetimes, in it at different program points.
> 
> - In contrast, the purpose of this patch (https://reviews.llvm.org/D16403) is 
> to add markers for when the storage duration for the variable begins and ends 
> (this is, when the storage exists). Once the storage duration has ended, you 
> can't placement new into the variables address, because another variable may 
> already be at that address.
> 
> I don't think there is an "issue" to resolve here.  We should make sure the 
> two patches play nicely with each other, though. In particular, we should 
> make sure that the markers for when lifetime ends and when storage duration 
> ends are ordered correctly.


What I wanted to add, I wonder if we might not get ScopeEnd event for 
temporaries and there might be other differences. The Clang implementation of 
P1179 used LifetimeEnd instead of ScopeEnd, and I believe probably most clients 
are more interested in LifetimeEnd events rather than ScopeEnd.

I think I understand why you went with ScopeEnd for this specific problem, but 
to avoid the confusion from having both in the Cfg (because I anticipate 
someone will need LifetimeEnd at some point), I wonder if we can make this work 
with LifetimeEnd. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149144

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

[PATCH] D149464: [clang][dataflow] Expose DataflowAnalysisContext from DataflowEnvironment.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:189
 
+  /// Deprecated. Use *getDataflowAnalysisContext().getOptions().Log instead.
   Logger () const { return *DACtx->getOptions().Log; }

bazuzi wrote:
> xazax.hun wrote:
> > Any reason for a comment as opposed to the deprecated attribute? 
> I couldn't tell exactly what the conventions were or whether all of the 
> chained replacements could be used as the FIX very well.
> 
> If you can confirm that `LLVM_DEPRECATED("DataflowAnalysisContext is now 
> directly exposed.", "*getDataflowAnalysisContext().getOptions().Log")`, 
> `LLVM_DEPRECATED(..., "getDataflowAnalysisContext().arena")`, etc. will 
> provide useful fixes, happy to replace with those. Or with replacements just 
> in the MSG and "" for FIX otherwise.
Unfortunately, I am also not 100% sure what is the convention. But I am OK with 
not providing a FIXIT here. I do not expect too many clients yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149464

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

[PATCH] D149464: [clang][dataflow] Expose DataflowAnalysisContext from DataflowEnvironment.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:189
 
+  /// Deprecated. Use *getDataflowAnalysisContext().getOptions().Log instead.
   Logger () const { return *DACtx->getOptions().Log; }

Any reason for a comment as opposed to the deprecated attribute? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149464

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

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

I am a bit overloaded at the moment, feel free to commit. I can still add 
comments later that could be addressed in a follow up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591

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

[PATCH] D148344: [clang][dataflow] Refine matching of optional types to anchor at top level.

[Gábor Horváth via Phabricator via cfe-commits]

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

LG! Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148344

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

[PATCH] D148344: [clang][dataflow] Refine matching of optional types to anchor at top level.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:262
 /// Returns true if and only if `Type` is an optional type.
 bool isOptionalType(QualType Type) {
   if (!Type->isRecordType())

Why do we need two places to define what is an optional type? Would it be 
possible to somehow reuse the Declaration matcher on the RecordDecel here? Or 
would that be bad for performance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148344

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

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:77
+
+void escape(char C, llvm::raw_ostream ) {
+  switch (C) {

We already sort of have a way to escape HTML here: 
https://github.com/llvm/llvm-project/blob/132f1d31fd66c30baf9773bf8f37b36a40fa7039/clang/include/clang/Rewrite/Core/HTMLRewrite.h#L60

I think we should reuse that (and potentially extend it if it does not fit our 
needs).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591

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

[PATCH] D147302: [clang][dataflow] Add `create()` methods to `Environment` and `DataflowAnalysisContext`.

[Gábor Horváth via Phabricator via cfe-commits]

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



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:100
+// used `StorageLocation` subclasses and make them use a 
`BumpPtrAllocator`.
+Locs.push_back(std::make_unique(std::forward(args)...));
+return *cast(Locs.back().get());

Would emplace back work? That returns a reference to the just emplaced element 
saving us the call to `back` making the code a bit more concise.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:328
+  template 
+  std::enable_if_t::value, T &>
+  create(Args &&...args) {

Just curious, what is the reason for repeating the `enable_if` here in addition 
to the one in the called function? Do we get better error messages?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147302

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

[PATCH] D147326: [clang][dataflow][NFC] Share code between Environment ctor and pushCallInternal().

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:465
+  /// referenced in `FuncDecl`. `FuncDecl` must have a body.
+  void initVars(const FunctionDecl *FuncDecl);
 

I wonder if we should rename this to something like `initFieldAndGlobals`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147326

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

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D146591#4213614 , @sammccall wrote:

> (e.g. does the MLIR dataflow framework have the same idea about how the 
> analysis timeline relates to the CFG and elements within it? And if we want 
> to show the clang AST or the StorageLocation/Value graph, what's the 
> non-clang::dataflow-coupled way to model that?)

These are good questions.

In D146591#4213614 , @sammccall wrote:

> Do you think that's valuable?

In case it does not add too much complexity on your side I think it is 
valuable. In case someone wants to use the same visualization for their tool it 
would be on them to figure out how to generalize the visualization part. On the 
other hand, even if we never get new users for the visualization, a JSON file 
will make it easier to build new diagnostic tools for the dataflow framework. 
One could even do one-off python scripts to help debugging a specific issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591

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

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/Logger.cpp:17
+Logger ::null() {
+  struct NullLogger : Logger {};
+  static auto *Instance = new NullLogger();

Adding `final`? Just in case it can help with devirtualization. 



Comment at: clang/lib/Analysis/FlowSensitive/Logger.cpp:23
+namespace {
+struct TextualLogger : Logger {
+  llvm::raw_ostream 

`final`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144730

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

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:186
+  Logger () const {
+return DACtx->getOptions().Log ? *DACtx->getOptions().Log : Logger::null();
+  }

If we already have a `NullLogger`, I wonder if making `DACtx->getOptions().Log` 
a reference that points to NullLogger when logging is disabled would be less 
confusing (and fewer branches).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144730

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

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

While I am OK with this approach, I wonder if it would be too much work to 
split the HTML generation and emitting data up. E.g., the logger could emit 
YAML or JSON that could be parsed by a python script to create the HTML (or the 
HTML itself could use JS to parse it and generate the DOM).
This would have a couple of advantages like:

- People could add additional visualizations or other tools like querying 
program states more easily just consuming the JSON/YAML file
- Other static analysis tools could produce similar YAML/JSON so your 
visualization could potentially work for other analysis tools. E.g., I think 
people were working on a dataflow analysis framework for MLIR.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591

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

[PATCH] D146514: [clang][dataflow] Fix crash when RHS of `&&` or `||` calls `noreturn` func.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D146514#4212528 , @mboehme wrote:

> No, I think this is a different case.

Sorry, I might have been unclear. I am 100% agree that these are different 
cases. I was wondering whether we can/should have a single code path supporting 
both. But I do not insist since we do not seem to have precedent for the 
scenario I mentioned and do not want to block anything based on hypotheticals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146514

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

[PATCH] D146507: [clang][dataflow][NFC] Eliminate StmtToEnvMap interface.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146507

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

[PATCH] D146538: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists

[Gábor Horváth via Phabricator via cfe-commits]

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

Ugh :/

I wonder if we should also open tickets on GitHub to reduce the chance of 
forgetting addressing the root cause for these. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146538

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

[PATCH] D146514: [clang][dataflow] Fix crash when RHS of `&&` or `||` calls `noreturn` func.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

This fix looks good for me for this particular problem, but I wonder whether 
the solution is general enough. In case the analysis figures out that a call 
would not return (e.g., the `value` method is called on a provably empty 
optional, and it would throw instead of returning), would this approach still 
work? Would the analysis update the `BlockReachable` bitvector on demand?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146514

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

[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Do you plan to selectively enable warnings coming from the STL to catch misuses 
of certain STL types?

I think we should probably discuss this direction first. It is, in general, a 
fragile approach. Users might use Clang with GCC's, LLVM's, or MSVC's standard 
library. Today, we do not have the means to ensure that these checks continue 
to work as expected on all STL implementations over time. Also, those warning 
reports might be leaky in a sense the reported path might contain 
implementations details from the STL that is hard to interpret.

I am afraid, if we want to provide a good user experience, we might be doomed 
to manually simulate the behavior of STL classes.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145069

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

[PATCH] D135495: [clang-tidy] handle exceptions properly in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I think at this point it is ok to merge. Any other comments can be addressed in 
follow-up commits.


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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

Thanks!


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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Overall, I like the structure of this patch and the references back to the 
standard. But every time we compare type pointers, they might compare inequal 
when one of the types have sugar on it while the other does not. Please review 
all of those comparisons to see where do we need to get the canonical types 
instead, and add some tests with type aliases.




Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:66
+   CXXBasePath &) {
+if (BS->getType()->getAsCXXRecordDecl() == BaseClass &&
+BS->getAccessSpecifier() == AS_public) {

Will this work with typedefs and other sugar or do you need to get the 
canonical type here? I do not see test coverage for that scenario.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:84
+
+QualType getPointeeOrArrayElementQualType(QualType T) {
+  if (T->isAnyPointerType())

What about `Type::getPointeeOrArrayElementType`?


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

https://reviews.llvm.org/D135495

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

[PATCH] D138037: [analyzer] Remove unjustified assertion from EQClass::simplify

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I am ok with committing this to unblock people hitting this assert, but at the 
same time I wonder if we want to open a ticket on GitHub that we might want to 
rethink how some of this works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138037

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

[PATCH] D143750: [clang-tidy] Clarify documention of `bugprone-unchecked-optional-access`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst:278
+
+Given that ``value()`` has well-defined program termination behavior, why treat
+it the same as ``operator*()`` which causes undefined behavior (UB)? That is,

Strictly speaking, `value` on an empty optional will not terminate the program. 
It will raise an exception that can be handled.

(Of course, at Google, exceptions are banned so termination is right in that 
context.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143750

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

[PATCH] D142710: [clang][dataflow] Relax validity assumptions in `UncheckedOptionalAccessModel`.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D142710#4096325 , @ymandel wrote:

> In D142710#4094934 , @xazax.hun 
> wrote:
>
>> This change looks good to me. I wonder, however, whether the behavior should 
>> be parameterized in the future. E.g., whether the user of the analysis 
>> should be able to make a decision whether the analysis should be pessimistic 
>> or optimistic about unmodeled values.
>
> Interesting idea. I think this goes along with other places where we are 
> unsound. Here, we err on the side of soundness. but, in general, we should 
> have a configuration mechanism for this.  FWIW, the only reason we have 
> uninitialized values at this point is recursive types. We also limit the 
> depth of structs, but that should be removed given my recent patch to only 
> model relevant fields. I have an idea for lazy initialization of values that 
> I think could solve the recursion issue. Together, we could remove this 
> concept of unmodeled values altogether from the framework.

Oh, sounds great! I do think lazy initialization will be really valuable to 
reduce the number of unmodeled values, but not entirely sure if we can 
completely eliminate them. In case we end up creating new locations (different 
from the earlier ones) in every iteration of the loop it might be harder to 
reach a fixed point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142710

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

[PATCH] D140897: [clang][dataflow] Fix handling of `DeclRefExpr`s to `BindingDecl`s.

[Gábor Horváth via Phabricator via cfe-commits]

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

I hope we will be able to get rid of this `SkipPast` thing at some point by 
looking at the value categories of the AST instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140897

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

[PATCH] D142710: [clang][dataflow] Relax validity assumptions in `UncheckedOptionalAccessModel`.

[Gábor Horváth via Phabricator via cfe-commits]

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

This change looks good to me. I wonder, however, whether the behavior should be 
parameterized in the future. E.g., whether the user of the analysis should be 
able to make a decision whether the analysis should be pessimistic or 
optimistic about unmodeled values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142710

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

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Overall looks good to me, I wonder if the tests could be less manual though. 
E.g., instead of asserting true/false, checking if the assignment would 
compile. This way we can be sure that the method in ASTContext matches the 
behavior of the compiler (and we get notified when the two diverge). If we 
could extract the corresponding code from Sema, it would be even better, but I 
do not insist as that might be a lot of work depending on how it interacts with 
other conversions.


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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a reviewer: erichkeane.
xazax.hun added a comment.

Since now the patch touches Clang proper, adding one more reviewer.


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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:81
+  FromPointeeTy->getUnqualifiedDesugaredType();
+  const Type *ToPointeeUQTy = ToPointeeTy->getUnqualifiedDesugaredType();
+

Here I am also wondering if we actually want canonical types.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+// At every level that array type is involved in, at least

isuckatcs wrote:
> xazax.hun wrote:
> > isuckatcs wrote:
> > > xazax.hun wrote:
> > > > If none of the functions I recommended works for you, I still strongly 
> > > > suggest reusing utilities from ASTContext, like `UnwrapSimilarTypes` 
> > > > and `UnwrapSimilarArrayTypes`.
> > > I didn't check all of the functions inside `ASTContext`, but here we have 
> > > very specific rules, we need to check. One utility might help with one 
> > > check or two, but won't replace the need to go ove all of them. Also I 
> > > think it's easier to understand which section checks what, if I keep them 
> > > separated.
> > > 
> > > In an ealier comment I link to [[ 
> > > https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions
> > >  | the section on cppreference ]], which discusses what these rules are. 
> > > Also there I found one controversial example, that's only detected by 
> > > MSVC. I wonder if you know why that happens.
> > I see. It is unfortunate that we cannot simply reuse the corresponding 
> > functionality from Sema. The code mostly looks good to me, apart from some 
> > nits.
> > It is unfortunate that we cannot simply reuse the corresponding 
> > functionality from Sema.
> 
> It is indeed unfortunate, though I wonder if we want to move this 
> functionality to `ASTContext`, so that it can be reused outside the 
> `ExceptionAnalyzer`.
I am not opposed to that, but I think in that case we want this method to be 
used in the regular compilation pipeline to make sure it is correct. 


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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Also, could you open a bug report about the strange exception behavior on 
GitHub? Hopefully someone working on conformance can take a look.


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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60
+// Checks if T1 is convertible to T2.
+static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2,
+   unsigned CurrentLevel = 0,

isuckatcs wrote:
> xazax.hun wrote:
> > xazax.hun wrote:
> > > Did you take a look at `ASTContext::hasCvrSimilarType`? I wonder if it is 
> > > already doing what you want. 
> > Oh, maybe it is not, but it might also make sense to take a look at 
> > `ASTContext::typesAreCompatible`.
> > Did you take a look at ASTContext::hasCvrSimilarType? I wonder if it is 
> > already doing what you want.
> 
> This function compares the types by removing the CVR qualifiers.
> 
> > Oh, maybe it is not, but it might also make sense to take a look at 
> > ASTContext::typesAreCompatible.
> 
> This one only compares the canonical types if the language is C++.
> 
> ```lang=c++
> if (getLangOpts().CPlusPlus)
> return hasSameType(LHS, RHS);
> 
> bool hasSameType(QualType T1, QualType T2) const {
> return getCanonicalType(T1) == getCanonicalType(T2);
>   }
> 
> ```
Could you rename the arguments like `To`, `From` or `Exception`, `Catch` or 
something to make the direction of the conversion clearer?



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+// At every level that array type is involved in, at least

isuckatcs wrote:
> xazax.hun wrote:
> > If none of the functions I recommended works for you, I still strongly 
> > suggest reusing utilities from ASTContext, like `UnwrapSimilarTypes` and 
> > `UnwrapSimilarArrayTypes`.
> I didn't check all of the functions inside `ASTContext`, but here we have 
> very specific rules, we need to check. One utility might help with one check 
> or two, but won't replace the need to go ove all of them. Also I think it's 
> easier to understand which section checks what, if I keep them separated.
> 
> In an ealier comment I link to [[ 
> https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions
>  | the section on cppreference ]], which discusses what these rules are. Also 
> there I found one controversial example, that's only detected by MSVC. I 
> wonder if you know why that happens.
I see. It is unfortunate that we cannot simply reuse the corresponding 
functionality from Sema. The code mostly looks good to me, apart from some nits.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:111
+// ... [or there is an array type of known bound in P1 and
+// an array type of unknown bound in P2 (since C++20)] then
+// there must be a 'const' at every single level (other than

If this rule only applies to C++20 and above, consider adding a language 
version check for LangOpts.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:130-131
+  if (!P1->isPointerType() || !P2->isPointerType())
+return convertible && P1->getUnqualifiedDesugaredType() ==
+  P2->getUnqualifiedDesugaredType();
+

Looking at the documentation of DesugaredType:
```
This takes off typedefs, typeof's etc. If the outer level of the type is 
already concrete, it returns it unmodified. This is similar to getting the 
canonical type, but it doesn't remove all typedefs. For example, it returns 
"T*" as "T*", (not as "int*"), because the pointer is concrete.
```
I wonder if we actually want to compare canonical types rather than desugared 
types.


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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+// At every level that array type is involved in, at least

If none of the functions I recommended works for you, I still strongly suggest 
reusing utilities from ASTContext, like `UnwrapSimilarTypes` and 
`UnwrapSimilarArrayTypes`.


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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60
+// Checks if T1 is convertible to T2.
+static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2,
+   unsigned CurrentLevel = 0,

xazax.hun wrote:
> Did you take a look at `ASTContext::hasCvrSimilarType`? I wonder if it is 
> already doing what you want. 
Oh, maybe it is not, but it might also make sense to take a look at 
`ASTContext::typesAreCompatible`.


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

https://reviews.llvm.org/D135495

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