Page MenuHomePhabricator

martong (Gabor Marton)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 10 2017, 8:01 AM (158 w, 2 d)

Recent Activity

Tue, Oct 20

martong added a comment to D89638: [Analyzer][WebKit] Add attributes to suppress specific checkers.

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

Honestly, I think that "CSA:supress:RefCntblBaseVirtualDtor" is way too verbose. It is incredibly easy to make mistakes, it seems hard to use and it is hell a lot to put in the actual code. It also doesn't resolve the issue with what unique identifier we should use. Imagine we decide that we should use Bug type as that unique identifier. It means that "CSA:supress:RefCntblBaseVirtualDtor" should get changed, but it is already in the user's code! It is quite important to settle on a solution that will still be available when we land a more general solution.

Tue, Oct 20, 8:50 AM
martong added a comment to D89638: [Analyzer][WebKit] Add attributes to suppress specific checkers.

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

Tue, Oct 20, 8:06 AM
martong accepted D89528: [clang][test] Fix prefix operator++ signature in iterators.

Ok, thanks for the context. LG!

Tue, Oct 20, 5:14 AM · Restricted Project
martong abandoned D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex.
Tue, Oct 20, 4:41 AM · Restricted Project
martong reopened D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex.
Tue, Oct 20, 4:41 AM · Restricted Project
martong added a comment to D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex.

Unfortunately, I could not reproduce the mentioned crash on our macOS machine. The mentioned test just passed with the output below. I gave up.

myuser@msmarple ~/llvm3/build/release_assert $ /usr/local/Frameworks/Python.framework/Versions/3.8/bin/python3.8 /Users/myuser/llvm3/git/llvm-project/lldb/test/API/dotest.py -S nm -u CXXFLAGS -u CFLAGS --codesign-identity - --env LLVM_LIBS_DIR=/Users/myuser/llvm3/build/release_assert/./lib --arch x86_64 --build-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex -s /Users/myuser/llvm3/build/release_assert/lldb-test-traces --lldb-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /Users/myuser/llvm3/build/release_assert/./bin/lldb --compiler /Users/myuser/llvm3/build/release_assert/./bin/clang --dsymutil /Users/myuser/llvm3/build/release_assert/./bin/dsymutil --filecheck /Users/myuser/llvm3/build/release_assert/./bin/FileCheck --yaml2obj /Users/myuser/llvm3/build/release_assert/./bin/yaml2obj --server /Users/myuser/llvm3/build/release_assert/./bin/debugserver --lldb-libs-dir /Users/myuser/llvm3/build/release_assert/./lib /Users/myuser/llvm3/git/llvm-project/lldb/test/API/commands/expression/import_builtin_fileid/ -p TestImportBuiltinFileID.py -t
lldb version 12.0.99 (https://github.com/llvm/llvm-project.git revision d454328ea88562a6ec6260529a040035ab9c4a06)
  clang revision d454328ea88562a6ec6260529a040035ab9c4a06
  llvm revision d454328ea88562a6ec6260529a040035ab9c4a06
libstdcxx tests will not be run because: Don't know how to build with libstdcxx on macosx
Skipping following debug info categories: ['dwo']
Tue, Oct 20, 4:39 AM · Restricted Project

Mon, Oct 19

martong added a reviewer for D88859: APINotes: add APINotesYAMLCompiler: NoQ.

Adding @NoQ as a reviewer, he might have some valuable comments, as he is very keen on this feature.

Mon, Oct 19, 9:53 AM · Restricted Project
martong added a comment to D88859: APINotes: add APINotesYAMLCompiler.

as it is something which is actually in production already
...
At the very least we need it for compatibility - this is already a shipping feature.

Mon, Oct 19, 9:49 AM · Restricted Project

Fri, Oct 16

martong added a comment to D89528: [clang][test] Fix prefix operator++ signature in iterators.

What is the context here? Did it cause any crash/bug or were you just reading through the code for a good night sleep? :D

Fri, Oct 16, 7:08 AM · Restricted Project
martong added a comment to D88859: APINotes: add APINotesYAMLCompiler.

Seems like this patch handles already existing attributes in Clang, so this might not be affected by the concerns brought up here by James:
http://lists.llvm.org/pipermail/cfe-dev/2020-October/066996.html

Fri, Oct 16, 5:55 AM · Restricted Project

Thu, Oct 15

martong added a comment to D63640: [clang] Improve Serialization/Imporing of APValues.

Reverse ping: I have a patch implementing class type non-type template parameters

that's blocked on this landing. If you won't have time to address @martong's comments soon, do you mind if I take this over and land it?

nice to see this coming.

It is okay for me to commit this patch in its current state. The changes I suggested could result in a cleaner code, but I can do those changes after we land this.

i couldn't apply martong's suggestion completely because importChecked is part of ASTNodeImporter not ASTImporter, but cleaned up some code.

Thu, Oct 15, 2:20 AM · Restricted Project

Wed, Oct 14

martong added a comment to D63640: [clang] Improve Serialization/Imporing of APValues.

Reverse ping: I have a patch implementing class type non-type template parameters that's blocked on this landing. If you won't have time to address @martong's comments soon, do you mind if I take this over and land it?

Wed, Oct 14, 11:35 PM · Restricted Project
martong added a comment to D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex.

Ups, sorry for the break and thanks for the notice! I am going to investigate.

Wed, Oct 14, 11:25 PM · Restricted Project
martong resigned from D66782: SourceManager: Prefer Optional<MemoryBufferRef> over MemoryBuffer*.
Wed, Oct 14, 5:41 AM · Restricted Project
martong committed rG73c6beb2f705: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex (authored by martong).
[ASTImporter] Fix crash caused by unset AttributeSpellingListIndex
Wed, Oct 14, 5:11 AM
martong closed D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex.
Wed, Oct 14, 5:11 AM · Restricted Project
martong added a comment to D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex.

Thanks for the reivew!

Wed, Oct 14, 5:10 AM · Restricted Project
martong added a comment to D89319: [ASTImporter] Fix crash caused by unimported type of FromatAttr.

Thanks for the review!

Wed, Oct 14, 4:55 AM · Restricted Project
martong committed rGdd965711c9f0: [ASTImporter] Fix crash caused by unimported type of FromatAttr (authored by martong).
[ASTImporter] Fix crash caused by unimported type of FromatAttr
Wed, Oct 14, 4:55 AM
martong closed D89319: [ASTImporter] Fix crash caused by unimported type of FromatAttr.
Wed, Oct 14, 4:55 AM · Restricted Project

Tue, Oct 13

martong requested review of D89319: [ASTImporter] Fix crash caused by unimported type of FromatAttr.
Tue, Oct 13, 7:13 AM · Restricted Project
martong requested review of D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex.
Tue, Oct 13, 7:12 AM · Restricted Project

Mon, Oct 5

martong added a comment to D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl.

Currently, we are totally inconsistent about the diagnostics. Either we should emit a diagnostic before all return false or we should not ever emit any diags. The diagnostics in their current form are misleading, because there could be many notes missing. I am not sure how much do you guys value these diags in LLDB, but in CTU we simply don't care about them. I'd rather remove these diags from the equivalency check code, because they are causing only confusion. (Or we should properly implement and test all aspects of the diags.)

Mon, Oct 5, 5:22 AM · Restricted Project
martong committed rG007dd12d5468: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl (authored by martong).
[ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl
Mon, Oct 5, 5:22 AM
martong closed D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl.
Mon, Oct 5, 5:22 AM · Restricted Project

Fri, Oct 2

martong added inline comments to D63640: [clang] Improve Serialization/Imporing of APValues.
Fri, Oct 2, 7:01 AM · Restricted Project
martong requested changes to D63640: [clang] Improve Serialization/Imporing of APValues.

Sorry for the late review, I just noticed something which is not a logical error, but we could make the ASTImporter code much cleaner.

Fri, Oct 2, 6:44 AM · Restricted Project
martong updated the diff for D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl.
  • Delegate to BitWidth Expr matching in case of BitFields
Fri, Oct 2, 5:11 AM · Restricted Project
martong added a comment to D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl.

So was the bug we were saying there were falsely equivalent or falsely not equivalent?

I think the bug is the crash :)

Yes. More specifically, the call of getBitWidthValue() caused a segfault with release builds and with assertions enabled it caused the mentioned assert on the given test code.

Fri, Oct 2, 5:11 AM · Restricted Project

Thu, Oct 1

martong added a comment to D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl.

The added test causes the below assertion in the baseline (w/o the fix):

ASTTests: ../../git/llvm-project/clang/lib/AST/ExprConstant.cpp:14543: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext&, llvm::SmallVectorImpl<std::pair<clang::SourceLocation, clang::PartialDiagnostic> >*) const: Assertion `!isValueDependent() && "Expression evaluator can't be called on a dependent expression."' failed.
Thu, Oct 1, 8:51 AM · Restricted Project
martong requested review of D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl.
Thu, Oct 1, 8:49 AM · Restricted Project

Wed, Sep 30

martong added a comment to D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.

What are our options mitigating anything similar happening in the future?

This way any change touching the SymbolicRangeInferrer and any related parts of the analyzer seems to be way too fragile.
Especially, since we might want to add support for comparing SymSyms, just like we try to do in D77792.

What about changing the EXPENSIVE_CHECKS in the assume function in the following way:
Convert all range constraints into a Z3 model and check if that is UNSAT.
In that case, we would have returned a state with contradictions, so we would prevent this particular bug from lurking around to bite us later.

And another possibility could be to create a debug checker, which registers to the assume callback and does the same conversion and check.
This is more appealing to me in some way, like decouples the Z3 dependency from the ConstraintManager header.

Which approach should I prefer? @NoQ @vsavchenko @martong @xazax.hun @Szelethus

Wed, Sep 30, 5:09 AM · Restricted Project

Tue, Sep 29

martong added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.

In this example, it cast to the unsigned char (which is the type of the stored value of **b, stored at #1) instead of the static type of the access (the type of **b is charat #2).

Tue, Sep 29, 7:10 AM · Restricted Project

Fri, Sep 25

martong added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

Beware, Phabricator ruins the visual experience of this nice analysis. E.g //char ***// is visible as an italic char *.

Fri, Sep 25, 2:41 AM · Restricted Project

Sep 23 2020

martong committed rG11d2e63ab006: [analyzer][StdLibraryFunctionsChecker] Separate the signature from the summaries (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Separate the signature from the summaries
Sep 23 2020, 2:02 AM
martong closed D88100: [analyzer][StdLibraryFunctionsChecker] Separate the signature from the summaries.
Sep 23 2020, 2:02 AM · Restricted Project
martong committed rGd63a945a1304: [analyzer][StdLibraryFunctionsChecker] Fix getline/getdelim signatures (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Fix getline/getdelim signatures
Sep 23 2020, 1:59 AM
martong closed D88092: [analyzer][StdLibraryFunctionsChecker] Fix getline/getdelim signatures.
Sep 23 2020, 1:59 AM · Restricted Project
martong added a comment to D88092: [analyzer][StdLibraryFunctionsChecker] Fix getline/getdelim signatures.

A joy of reviewing C++ code is that you get to marvel in all the great things the language has, without having to pull fistfuls of hair out to get get to that point. These patches are always a treat. LGTM!

Sep 23 2020, 1:42 AM · Restricted Project
martong added a comment to D88100: [analyzer][StdLibraryFunctionsChecker] Separate the signature from the summaries.

Thanks for the review! :)

Sep 23 2020, 1:41 AM · Restricted Project

Sep 22 2020

martong requested review of D88100: [analyzer][StdLibraryFunctionsChecker] Separate the signature from the summaries.
Sep 22 2020, 8:28 AM · Restricted Project
martong requested review of D88092: [analyzer][StdLibraryFunctionsChecker] Fix getline/getdelim signatures.
Sep 22 2020, 6:04 AM · Restricted Project

Sep 21 2020

martong added a reviewer for D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs: vsavchenko.

Adding Valeriy as a reviewer. He's been working with the solver recently, so his insights might be really useful here.

Sep 21 2020, 8:22 AM · Restricted Project
martong committed rG0c4f91f84b2e: [analyzer][solver] Fix issue with symbol non-equality tracking (authored by martong).
[analyzer][solver] Fix issue with symbol non-equality tracking
Sep 21 2020, 8:00 AM
martong closed D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.
Sep 21 2020, 8:00 AM · Restricted Project
martong updated the diff for D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.
  • Avoid same pattern when checking whether the assumption is infeasible
Sep 21 2020, 7:33 AM · Restricted Project
martong added a comment to D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.

Thanks for the quick review!
I updated according to your suggestion, so we avoid the same pattern.

Sep 21 2020, 7:33 AM · Restricted Project
martong added a comment to D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.

@steakhal, thank you for your time and huge effort in debugging this!

Sep 21 2020, 4:47 AM · Restricted Project
martong added a comment to D82445: [analyzer][solver] Track symbol equivalence.

We came up with a quick fix, let us know what you think:
https://reviews.llvm.org/D88019

Sep 21 2020, 4:46 AM · Restricted Project
martong requested review of D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.
Sep 21 2020, 4:45 AM · Restricted Project
martong abandoned D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash.
Sep 21 2020, 2:25 AM · Restricted Project
martong updated subscribers of D82445: [analyzer][solver] Track symbol equivalence.

Hi Valery,

Sep 21 2020, 1:16 AM · Restricted Project

Sep 17 2020

martong accepted D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl.
Sep 17 2020, 10:36 PM · Restricted Project, Restricted Project
martong added a comment to D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash.

Alright, I refactored the change a bit. A Constraint::assume works similarly to an engine DualAssume. Plus I added isApplicable to check if it is meaningful to call the assume at all.

Sep 17 2020, 10:03 AM · Restricted Project
martong updated the diff for D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash.
  • apply -> assume, Add isApplicable
Sep 17 2020, 10:01 AM · Restricted Project
martong added inline comments to D79432: [analyzer] StdLibraryFunctionsChecker: Add summaries for libc.
Sep 17 2020, 6:37 AM · Restricted Project
martong accepted D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl.

We are going to move forward with this approach (after dealing with the multi-dimensional array case) temporarily. We are seeing crash bugs from this from users and we want to fix it while we explore the solution space more.

Okay, I understand the business reasons and I am fine with this patch if this is really a temporary solution. Just please add a FIXME that indicates this and D71378 are temporary, plus that this should be really handled by the preceding import call.

Sep 17 2020, 12:10 AM · Restricted Project, Restricted Project

Sep 16 2020

martong added inline comments to D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash.
Sep 16 2020, 12:13 PM · Restricted Project
martong requested review of D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash.
Sep 16 2020, 12:11 PM · Restricted Project
martong accepted D87619: [ASTImporter] Refactor IsStructurallyEquivalent's Decl overloads to be more consistent.

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

Sep 16 2020, 12:25 AM · Restricted Project

Sep 15 2020

martong added a comment to D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite.

Actually, the test added here could have catched that regression.

Sep 15 2020, 10:02 PM · Restricted Project
martong added a comment to D87683: [clang-tidy] Crash fix for bugprone-misplaced-pointer-arithmetic-in-alloc.

Please fix the test case first, can't call operator new(unsigned long, void*) with an argument of type void*
The other failures the pre merge bot detected can safely be disregarded

Placement new is defined per standard:

void* operator new  ( std::size_t count, void* ptr );

Here, the problem is that size_t is unsigned long on Linux and it seems that it is unsigned long long on Windows. How should I overcome this?

Sep 15 2020, 11:30 AM · Restricted Project, Restricted Project
martong committed rGa012bc4c42e4: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite
Sep 15 2020, 7:37 AM
martong closed D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite.
Sep 15 2020, 7:36 AM · Restricted Project
martong added a comment to D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite.

There is a blatant regression we introduced in D87240. Actually, the test added here could have catched that regression.
See https://reviews.llvm.org/D87240#inline-812062
I am putting back the modeling dependency with this patch.

Sep 15 2020, 7:35 AM · Restricted Project
martong added inline comments to D87240: [analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies.
Sep 15 2020, 7:32 AM · Restricted Project
martong added a comment to D87619: [ASTImporter] Refactor IsStructurallyEquivalent's Decl overloads to be more consistent.

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

A related question: You see any problem with doing the same generated Traversal* code we do for the Stmts also for the Decls? This way we could simplify some common checks (e.g., NameDecl's name comparison could cover all subclasses that want it). It seems that for example the Records can't just reuse the name comparison because of the getTypedefNameForAnonDecl() trick, so it might not be as simple as the Stmt code.

Sep 15 2020, 2:23 AM · Restricted Project
martong added a comment to D87619: [ASTImporter] Refactor IsStructurallyEquivalent's Decl overloads to be more consistent.

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

Sep 15 2020, 1:55 AM · Restricted Project

Sep 14 2020

martong updated the diff for D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite.
  • Add tests to verify compatibility of the two checkers
Sep 14 2020, 10:14 AM · Restricted Project
martong added inline comments to D87519: [analyzer][Liveness][NFC] Enqueue the CFGBlocks post-order.
Sep 14 2020, 6:43 AM · Restricted Project
martong added inline comments to D87518: [analyzer][Liveness][NFC] Remove an unneeded pass to collect variables that appear in an assignment.
Sep 14 2020, 5:59 AM · Restricted Project
martong added a comment to D87518: [analyzer][Liveness][NFC] Remove an unneeded pass to collect variables that appear in an assignment.

This problem only exists if we traverse a CFGBlock in order. And Liveness in fact does it reverse order. So a distinct pass is indeed unnecessary, we can note the appearance of the assignment by the time we reach the variable.

Sep 14 2020, 5:55 AM · Restricted Project
martong accepted D87527: [ASTMatchers] Fix `hasBody` for the descendants of `FunctionDecl`.

Unfortunately template specializations do not catch the descendants of the class for which the template is specialized. Therefore it does not work correcly for the descendants of FunctionDecl, such as CXXMethodDecl, CXXConstructorDecl, CXXDestructorDecl etc. This patch fixes this issue by using a template metaprogram.

Sep 14 2020, 2:14 AM · Restricted Project

Sep 12 2020

martong added a comment to D87444: [ASTImporter] Add basic support for comparing Stmts and compare function bodies.

Probably it should be tested how this change affects CTU analysis (function body is checked now).

I queued a build (#1704) in our CI for CTU: http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/
I don't expect much regression because my gut feeling is that it is quite rare to compare two function definitions. But let's see the results. Raphael would you mind if we wait for the results before committing?

Sep 12 2020, 11:53 AM · Restricted Project

Sep 11 2020

martong added a comment to D87444: [ASTImporter] Add basic support for comparing Stmts and compare function bodies.

Probably it should be tested how this change affects CTU analysis (function body is checked now).

Sep 11 2020, 8:40 AM · Restricted Project
martong accepted D87444: [ASTImporter] Add basic support for comparing Stmts and compare function bodies.

Thank you for the changes! This looks good to me now!

Sep 11 2020, 5:55 AM · Restricted Project
martong edited reviewers for D87444: [ASTImporter] Add basic support for comparing Stmts and compare function bodies, added: balazske, a_sidorin; removed: a.sidorin, jdoerfert.

Changing reviewers:

  • Removing 'a.sidorin' with dot. Afaik, Alexei's live user is 'a_sidorin' with an underscore, so I added it.
  • Removing 'jdoerfert' because his herald script is ought to match OpenMP related changes, but it matched 'omp' in the word 'compare' in the title.
  • Adding 'balazske' as he's been working recently with this class. @balazske, if you have some time, please take a look, your insight could be useful.
Sep 11 2020, 12:21 AM · Restricted Project

Sep 10 2020

martong added a comment to D87444: [ASTImporter] Add basic support for comparing Stmts and compare function bodies.

Thanks! This patch is really cool, I like it overall.

Sep 10 2020, 8:59 AM · Restricted Project
martong committed rGa97648b93846: [analyzer][StdLibraryFunctionsChecker] Add better diagnostics (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Add better diagnostics
Sep 10 2020, 3:45 AM
martong closed D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics.
Sep 10 2020, 3:44 AM · Restricted Project
martong committed rGb7586afc4dcd: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Remove strcasecmp
Sep 10 2020, 3:32 AM
martong closed D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp.
Sep 10 2020, 3:31 AM · Restricted Project
martong added a comment to D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics.

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

I realize that the how and why phrases in this context a bit too vague :) What do you mean under having to dump the whole State? I didn't mean to compress a bug path into a warning message, only what I mentioned in D79431#2020951. In any case, I think its okay to just move on with this patch. LGTM!

Sep 10 2020, 1:59 AM · Restricted Project

Sep 9 2020

martong added a comment to D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics.

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.

edit: Describing how the violation happened might be a valuable for development purposes as well.

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

Sep 9 2020, 9:29 AM · Restricted Project
martong added a comment to D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics.

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.

edit: Describing how the violation happened might be a valuable for development purposes as well.

Sep 9 2020, 9:24 AM · Restricted Project
martong added a comment to D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp.

@steakhal Ping.

Sep 9 2020, 9:07 AM · Restricted Project

Sep 8 2020

martong accepted D76604: [Analyzer] Model `size()` member function of containers.

This basically looks good to me.

Sep 8 2020, 6:22 AM · Restricted Project
martong accepted D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__.

LGTM! Thanks for the clarification and the example you gave.
(I agree with @steakhal and I wish if we could get rid of the many lines not-descriptive plist stuff, but that is rather unrelated)

Sep 8 2020, 6:11 AM · Restricted Project
martong added inline comments to D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME.
Sep 8 2020, 5:39 AM · Restricted Project
martong added a comment to D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME.

But if the string is invalidated (or the new length is completely unknown), do not remove the length from the state; instead, set it to SymbolConjured.

Where exactly do you implement the above?

Sep 8 2020, 5:37 AM · Restricted Project

Sep 7 2020

martong added a comment to D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp.

I completely agree with you.
I plan to further refactor the CStringChecker, but the related patches are pretty much stuck :D

I think this workaround is fine for now.
You might as well extend the corresponding parts of the CStringChecker to make the modelling more precise.
It shouldn't be much of a hassle.
What do you say about that?

I think the modeling is well done and precise. I mean, it seems like all of the constraints that I am removing here are handled in CStringChecker. It checks the pointer arguments whether they are null. Also, the length is checked in case of strncasecmp, here:

if (IsBounded) {
  // Get the max number of characters to compare.
  const Expr *lenExpr = CE->getArg(2);
  SVal lenVal = state->getSVal(lenExpr, LCtx);
Sep 7 2020, 9:06 AM · Restricted Project
martong added a comment to D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite.

The patch looks great, in fact, it demonstrates how well thought out your summary crafting machinery is.

However, in a similar case with the CallAndMessage Checker, we decided to list the more specific Checker as a dependency.

We got the answer to D77061#2057063! We should turn it into a weak dependency though (D80905).

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition. The return value is constrained by StreamChecker too but it splits the error (if returned value is less that arg 3) and non-error cases into separate branches. I think this causes no problem because it will refine the assumption made here (if this assumption is made first) or the assumption here has no effect (if the split happened already).

Be sure to triple check whether the ExplodedGraph looks okay with both checkers enabled.

Sep 7 2020, 8:59 AM · Restricted Project
martong committed rG8248c2af9497: [analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies
Sep 7 2020, 8:57 AM
martong closed D87240: [analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies.
Sep 7 2020, 8:56 AM · Restricted Project
martong committed rGd01280587d97: [analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions
Sep 7 2020, 8:47 AM
martong closed D84415: [analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions.
Sep 7 2020, 8:47 AM · Restricted Project
martong requested review of D87240: [analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies.
Sep 7 2020, 8:21 AM · Restricted Project
martong requested review of D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp.
Sep 7 2020, 8:11 AM · Restricted Project
martong added a comment to D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite.

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition.

There is nothing new in that. This assumption described by the .Case has been here since the inception of this Checker. This patch does not change it. This patch adds two new argument constraints.

Sep 7 2020, 12:55 AM · Restricted Project

Sep 4 2020

martong committed rGf0b9dbcfc7ba: [analyzer][StdLibraryFunctionsChecker] Add POSIX time handling functions (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Add POSIX time handling functions
Sep 4 2020, 9:45 AM