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?)

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.

In D146514#4212528, @mboehme wrote:

No, I think this is a different case.

Thanks!

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?

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?

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

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

Thanks!

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.

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.

In D143328#4111595, @NoQ wrote:

I think the right extra data to include is the CFGElementRef to the destructor element.

Thanks! This is exactly what I had in mind! Let's wait for @NoQ just in case he had something different in mind.

I think this patch should also add a test that fails before your changes.

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.

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.

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.

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.

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

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

I have one nit that you should look into, otherwise sounds reasonable and looks good to me.

In D139534#4036783, @steakhal wrote:

In D139534#4034719, @xazax.hun wrote:

Here is the gist of one *new* TP:

Where would sprops get escaped? Did I miss that or was that reduced out of the example?

You are right, it 'never' escapes, yet in the past we modelled all stores to local statics as an 'immediate escape'.
This is what I think we should not do. And this is what this patch removes.

Here is the gist of one *new* TP:

I think in these cases you can recommit without waiting for a new round of reviews, but I hit accept just in case :)

Sorry, I got a bit swamped, will try to take a look next week. In the meantime, did you have a chance to run this over some open source projects? Did you find any interesting diffs?

I realized I did not say this explicitly in my previous comment, but feel free to commit :)

This is a big improvement, I skimmed through it and looks good to me. There might be some sneaky ways to change values of fields without mentioning them, e.g., casting a pointer to a struct to an array of chars. But I think reasoning about those scenarios is probably not something we want to model anytime soon.

I don't mind committing these patches, I have a high confidence in you going back and addressing feedback post-commit if something is coming up.

This approach looks good to me. Some context: we kept the CFGs lightweight because it looks like we did not need to do any extensions for the Clang Static Analyzer. I'm glad the dataflow framework can also work with the current representation. On the other hand, I think structured bindings in C++ are really limited, e.g., we cannot nest them, and it is nowhere near what pattern matching can do in other languages like Rust. I do remember seeing papers about extending structured bindings in at least some dimensions like nesting. I wonder if making the representation more explicit in the CFG as structured bindings get more complex will simplify implementations in the future, but that is a problem for tomorrow :)

LGTM!

I am a bit conflicted. It is unfortunate that C and C++ compilers regarded single element array members as flexible array members. On the other hand, looking at GCC, it recently added -fstrict-flex-arrays=2 as an option to no longer consider single element member arrays as FAM. So, it looks like the community wants to migrate away from this. My main concern is whether this option would make the experience worse for people who keep their code tidy and favor people who did not update their FAMs. Overall, I wonder if diagnosing single element arrays that are likely FAMs and suggesting users to fix their code is a better way forward.

I guess there are some more options. We could try keeping representatives alive no matter what. It could be a good exercise to see if doing that makes any difference in the analysis results.

I did not spend too much time thinking about this yet, but this sounds scary. I wonder if we should target the underlying problem instead, i.e., making sure we never have dead symbols as representatives for eq. classes. What do you think?

Sounds reasonable to me.

Is the problem forEachDescendant matching statements inside blocks and lambdas? I wonder if this behavior would surprise people, so I think it would be better to:

• Potentially add a template bool parameter to forEachDescendant controlling this behavior.
• Review existing uses because I am not entirely sure if the current behavior is the right default.

Overall, the document looks good to me, I like the general direction. I still see some pending comments (mostly small wording fixes) from Aaron.

LGTM, thanks!

Thanks, the latest version looks good to me.

Mostly looks good to me, but there still is a comment that is unaddressed.

Start to look good, I still have a couple of questions/comments inline.

With the rest of the comments addressed it looks good to me.

In D135965#3859442, @isuckatcs wrote:

We could also consider reporting a warning or an error in this case to let the caller know that the function doesn't work in this context. Right now it's up to the caller to figure it out on their own, and if they don't see the report they might think there's something wrong with some other part of their code.

I'd argue the warning is correct as you could store "non-enum" integer values into an enum typed variable (and people do that fairly often with bitwise enums).

Oh, I see, this look deliberate. I wonder if we want to add an exception for lambdas though but that would be a different discussions unrelated to this PR.

In D135965#3858855, @isuckatcs wrote:

Moved clang_analyzer_eval() out of the lambda body, because no diagnostic was reported from the inside.