This has been backported to clang-16.0.1 as 1172ed57d8234990b277281b3084969fcdb38602

Speaking of issue tracking, I think it would be much more valuable to our users if we had a proper release notes section for CSA. Right now it is so ad-hoc.
That is also true for crash fixes and backports. Maybe we should put more emphasis on that part for the next major release.
I'm not sure how to enforce it though.

In D146538#4210037, @xazax.hun wrote:

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?

I think this one also deserves backporting to clang-16.0.1 @xazax.hun

BTW we request full context for git patches. That way the reviewer can scroll and see the context around your hunks.
You can request git to emit the full context by passing the -U999999 commandline option. See.

Okay, thanks for clarifying.

Thanks for looking into this.

As this resolves a crash, I'm backporting it: https://github.com/llvm/llvm-project/issues/61245

Ah sorry that it took me so long to push this. I completely forgot about this one.

FYI the release schedule is here: https://llvm.org/docs/HowToReleaseLLVM.html#annual-release-schedule

In D144546#4171238, @ymandel wrote:

In D144546#4167161, @steakhal wrote:

By looking at the title, I get the impression that this fixes an assertion violation.
I also observed that this commit is part of main but not part of release/16.x, hence the clang-16 would be released without this fix.

I want to raise awareness of backporting crash fixes to llvm releases. IMO that's a good practice.
So my question is, should we backport this patch to the release branch?

If so, could you please check if there are more commits like this for the dataflow library @ymandel?

I used git log release/16.x..main --oneline clang/lib/{Analysis,AST,ASTMatchers,StaticAnalyzer} clang/include/clang/{Analysis,AST,ASTMatchers,StaticAnalyzer} | grep -i 'crash\|fix\|assert' to check for relevant commits.

Yes, this could be backported. I'm happy to look out for this in the future, but I don't seem to have release/16.x in my git repo -- please let me know how to pull that in. I'm only responsible for Analysis/FlowSensitive, but even just there not all fixes have those search terms in there commit message.

By looking at the title, I get the impression that this fixes an assertion violation.
I also observed that this commit is part of main but not part of release/16.x, hence the clang-16 would be released without this fix.

@usaxena95 @ilya-biryukov, @hans, I wonder if we should backport this change to release/16.x. Otherwise, clang-16 won't have this fix. WDYT?
I'm also a bit worried that we don't have tests. And that we have "unexpected" sideeffects like what you mentioned @hans.

Hi @tbaeder, I was looking for commits that mentions "fix" "assertion" or "crash" in the title, that are part of the main branch but not backported to release/16.x to be eventually released as clang-16.
I wonder what's the status of this and the related patches given that the commit that the summary mentions (490e8214fca48824beda8b508d6d6bbbf3d8d9a7) is present in release/16.x. I can also see that quite a few revert commits are also related to Interp.
It's probably the case that the mentioned change was removed from clang-16 by f6ea1af9a4b71d27de2dde629224af1220c5c85b.

I'm backporting this to clang-16.
https://github.com/llvm/llvm-project/issues/61149

Hm, it would be easier to use llvm::StringMap<decltype(std::monostate)> instead of std::nullopt_t, or some adhoc empty struct.

Oh, I forgot to mention why this change breaks the equality operator of llvm::StringSet. It's because the StringMap::operator== will try to compare the value as well, which is of type std::nullopt_t and that has no equality comparison operator.
Previously, the enum class NoneType has a default one.

This patch breaks llvm::StringSet equality.
The following code would no longer compile:

llvm::StringSet LHS;
llvm::StringSet RHS;
bool equal = LHS == RHS;

Such code might be used as gtest assertions like EXPECT_EQ(LHS, RHS).
@kazu Do you think we should address this by providing an equality operator for the StringSet?
I was thinking of adding this:

bool operator==(const StringSet &RHS) const {
  if (Base::size() != RHS.size())
    return false;

I think we should backport this to release/16.x.
Here is the ticket for it: https://github.com/llvm/llvm-project/issues/61115

I'm proposing to backport this fix to clang-16.
https://github.com/llvm/llvm-project/issues/61097

If we worry about having taint-related reports without a Note message explaining where the taint was introduced, we could just assert that in a BugReportVisitor at the finalizeVisitor() callback. I think such an assertion would make a lot of sense.
To achieve this, we could take the R.getNotes() and check if any of them refers to a specific one produced by the NoteTag callback for taint sources, let's say TaintSourceTag for that PathDiagnosticNotePiece.

In D144780#4154619, @danix800 wrote:

In D144780#4154487, @steakhal wrote:

I love it. Short, to the point. Thanks.
I think this is the right way to fix this.
Good job.
Can you merge the change? Or should I do it on you behalf?

I've no permission to merge, please do it for me. Thanks!

First and foremost, I want to deeply apologize about my rushed response. When I say that the Taint originated here note remained, I wrongly draw conclusions. Won't happen again.

I love it. Short, to the point. Thanks.
I think this is the right way to fix this.
Good job.
Can you merge the change? Or should I do it on you behalf?

I haven't checked the implementation, but fundamentally patching the TaintBugVisitor is not how we should improve the diagnostic for taint issues.
I saw that this patch is not about NoteTags, so I didn't go any further that point.

In D138037#4134504, @steakhal wrote:

I'm backporting this as #60834 to the 16.x branch.

In D138037#4129160, @xazax.hun wrote:

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.

I'm backporting this as #60834 to the 16.x branch.

In D143867#4121874, @Scarlet1ssimo wrote:

The test passes on main. Are you sure about the reproducer?

This happens on versions from release/10.x to release/15.x. Since 16.x, this file gets refactored and no such problem ever.
I'm new to llvm. Are we still responsible for older versions?

We don't plan to backport a change like this to previous versions.
Thanks for your report and investigation though.

I would also suggest backporting this to the release/16.x branch as that has already branched off. WDYT?

@NoQ @xazax.hun I plan to merge this by the end of the week if you have no objection.

In D143867#4121841, @Scarlet1ssimo wrote:

In D143867#4121835, @steakhal wrote:

Looks sensible to me.
Do you have a test for triggering the previous assertion?

I do have prepared a test case like:

int testA(void) { return 0/0; }
// RUN: %clang_analyze_cc1 -analyzer-checker=core %s -analyzer-output=sarif -o -

Notice you must put the buggy code at the very first line to trigger the assertion.

The test passes on main. Are you sure about the reproducer?
I copy-pasted your code as-is into a test file, but check-clang-analysis still passes.

Looks sensible to me.
Do you have a test for triggering the previous assertion?

LGTM

I would not mind less artificial-looking test code, but I'll let you decide if you want to make action about it.

I would be interested in some of the free-functions dealing with variants, such as std::visit(). https://godbolt.org/z/bbocrz4dG
I hope that's also on the radar.

I'm not using this docker file, so I don't mind changing it.
It probably won't hurt :D

Looks good. Thanks.

I'm not that confident around gtest macros, but this doesn't look harmful.
Thanks for taking the time and fixing it.

I'm very much interrested. I think you should define a mock standard variant header, that you could include from your tests.

Sorry, I don't have the time this week.

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?

In D139534#4029097, @xazax.hun wrote:

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?

Let me know if you have further concerns. @xazax.hun @NoQ

• Add two test cases demonstrating the false-positives this patch will introduce. These are semantically equivalent to the one mentioned in the comments by @NoQ.

Finally, I had some time to come back to this.
Thanks for taking the time for such a detailed response, kudos! @NoQ

Bitwidth was important because we should ideally cast smaller bitwidth type to bigger bitwidth type.
Consider if we have LHS(u8), RHS(i32), then without checking for bitwidth, we would be casting RHS's maxValue to LHS's type, which will result in lose of information and will not serve our purpose.

If you think we need that bitwidth check, why did you remove it?
I'd like to see test cases demonstrating what we are talking about and see if we want that behavior or not.

This test fails.

void testfoo(unsigned char u, signed int s) {
  if (u >= 253 && u <= 255 && s < INT_MAX - 2) {
    // u: [253, 254], s: [INT_MIN, INT_MAX - 2]
    clang_analyzer_eval(u != s); // expected-warning{{UNKNOWN}}
                                 // but returns TRUE
  }
}

In D112621#4004409, @steakhal wrote:

In D112621#4004372, @uabelho wrote:

Hi,

The following starts crashing with this patch:

clang -cc1 -analyze -analyzer-checker=core bbi-77010.c

It crashes with

bbi-77010.c:6:1: warning: non-void function does not return a value [-Wreturn-type]
}
^
clang: ../../clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1622: clang::ento::RangeSet (anonymous namespace)::SymbolicRangeInferrer::VisitBinaryOperator(clang::ento::RangeSet, clang::ento::RangeSet, clang::QualType): Assertion `!LHS.isEmpty() && !RHS.isEmpty() && "Both ranges should be non-empty"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Abort (core dumped)

bbi-77010.c140 BDownload

Thanks for the report. I'll fix it ASAP.
I think I'll replace the assertion with an early return.

BTW, was this uncovered by fuzzing? @uabelho

In D112621#4004372, @uabelho wrote:

Hi,

The following starts crashing with this patch:

clang -cc1 -analyze -analyzer-checker=core bbi-77010.c

It crashes with

bbi-77010.c:6:1: warning: non-void function does not return a value [-Wreturn-type]
}
^
clang: ../../clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1622: clang::ento::RangeSet (anonymous namespace)::SymbolicRangeInferrer::VisitBinaryOperator(clang::ento::RangeSet, clang::ento::RangeSet, clang::QualType): Assertion `!LHS.isEmpty() && !RHS.isEmpty() && "Both ranges should be non-empty"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Abort (core dumped)

bbi-77010.c140 BDownload

About spellings. In the summary you used 'lesser', I think as a synonym for 'smaller' or something like that. Anyway, not important.
Great stuff.

In D138037#4003121, @vabridgers wrote:

Hi @steakhal, thanks for the suggested change.
How we can help move this forward? From what I'm comprehending from the notes, perhaps we could try running this change through our internal systems level test and fuzzer. Unfortunately, I'd not be able to say more than "trust me, we saw no problems" if we see no problems. But if I do find additional cases I can make simplified reproducers and we can work on addressing those.
Best!

I appreciate that you are interested in tackling this. I do think it's a low-hanging fruit, so it's a good choice!

Thanks for going the extra mile to address this last thing. I really appreciate it.
I've got only a few minor comments and suggestions.

The StaticAnalyzer portion looks good to me AFAICT.

The fix feels suboptimal in readability but let it be whatever.

Really good progress. Nicestuff. I really appreciate the unittest.
However Ive got some minor comments there :D

I think we could settle on something like this:

   APSIntType getAPSIntType(QualType T) const {
+    if (T->isFixedPointType())
+      return APSIntType(Ctx.getIntWidth(T), T->isUnsignedFixedPointType());
+
     // For the purposes of the analysis and constraints, we treat atomics
     // as their underlying types.
     if (const AtomicType *AT = T->getAs<AtomicType>()) {
       T = AT->getValueType();
     }

Nice catch.

For the test cases where we should be able to prove the case but return Unknown instead, should be marked by a FIXME stating the expected value.
Approximating a concrete value with Unknown is (almost) always correct. So, in this case, you don't need to go and fix them unless you want to do the extra mile.

In D139195#3966675, @aaron.ballman wrote:

Previously, only void returning functions were considered for noreturn attribute candidates. This patch removes this artificial restriction.

C2x 6.7.12.6p6: The implementation should produce a diagnostic message for a function declared with a noreturn attribute that appears to be capable of returning to its caller.

p7 has an example showing [[noreturn]] int h(void); with the comment "Implementations are similarly encouraged to diagnose the declaration of h() because it appears capable of returning to its caller due to the non-void return type."

So is this really an artificial restriction? To me, putting the noreturn attribute on a function with a return type makes no sense whatsoever. The interface is saying "I promise that calling me will not return" and "when I return, this is the type of the value I will give you."

I haven't looked at the implementation. I only checked the tests and something is not quite right there. See my comments inline.
BTW the line-coverage is good. I found only two branches which I want to have tests for - see inline.

I already reviewed this internally. For me, it looks great.

BTW the change itself looks good to me.

Looks good.

In D112621#3915822, @manas wrote:

Ping

I proposed the D138657 and D138659 patches for getting rid of this flag.

Are you sure about the speedup?
When I looked at the implementation of the StringSwitch, it boiled down to an if-else chain under the hood.