User Details
- User Since
- Mar 7 2019, 1:49 AM (212 w, 2 d)
Thu, Mar 23
This has been backported to clang-16.0.1 as 1172ed57d8234990b277281b3084969fcdb38602
Wed, Mar 22
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.
Tue, Mar 21
I think this one also deserves backporting to clang-16.0.1 @xazax.hun
Thu, Mar 16
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.
Tue, Mar 7
As this resolves a crash, I'm backporting it: https://github.com/llvm/llvm-project/issues/61245
Mon, Mar 6
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
Fri, Mar 3
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
Mar 2 2023
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
Mar 1 2023
I'm proposing to backport this fix to clang-16.
https://github.com/llvm/llvm-project/issues/61097
Feb 27 2023
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.
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?
Feb 21 2023
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.
Feb 20 2023
Feb 17 2023
I'm backporting this as #60834 to the 16.x branch.
Feb 12 2023
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.
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?
Jan 27 2023
LGTM
Jan 26 2023
I would not mind less artificial-looking test code, but I'll let you decide if you want to make action about it.
Jan 25 2023
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.
Jan 24 2023
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.
Jan 23 2023
I'm very much interrested. I think you should define a mock standard variant header, that you could include from your tests.
Jan 12 2023
Jan 9 2023
Sorry, I don't have the time this week.
Jan 5 2023
Jan 3 2023
Jan 2 2023
Let me know if you have further concerns. @xazax.hun @NoQ
Dec 21 2022
- 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 } }
Dec 19 2022
Dec 17 2022
About spellings. In the summary you used 'lesser', I think as a synonym for 'smaller' or something like that. Anyway, not important.
Great stuff.
Dec 16 2022
I appreciate that you are interested in tackling this. I do think it's a low-hanging fruit, so it's a good choice!
Dec 15 2022
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.
Dec 13 2022
The StaticAnalyzer portion looks good to me AFAICT.
Dec 11 2022
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(); }
Dec 10 2022
Nice catch.
Dec 9 2022
Dec 7 2022
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.
Dec 2 2022
Nov 29 2022
Nov 28 2022
Nov 25 2022
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.
Nov 24 2022
BTW the change itself looks good to me.
Looks good.
Nov 23 2022
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.