Szelethus (Umann Kristóf)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 19 2017, 6:59 AM (70 w, 20 h)

Recent Activity

Yesterday

Szelethus updated the summary of D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions.
Wed, Nov 21, 6:15 PM
Szelethus added a comment to D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions.

I also have some very neat ideas how the split up should go, but I'd like to mature the idea in my head for just a bit longer.

Wed, Nov 21, 6:10 PM
Szelethus created D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions.
Wed, Nov 21, 6:03 PM
Szelethus added a comment to D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend.

Polite ping :)

Wed, Nov 21, 2:52 PM
Szelethus added a comment to D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded.

Ping, @xazax.hun, any objections?

Wed, Nov 21, 11:07 AM
Szelethus added a comment to D53076: [analyzer] WIP: Enhance ConditionBRVisitor to write out more information.

In the meanwhile we managed to figure out where the problem lays, so if you're interested, I'm going to document it here.

Wed, Nov 21, 5:31 AM · Restricted Project

Tue, Nov 20

Szelethus added inline comments to D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit..
Tue, Nov 20, 10:16 AM
Szelethus added a comment to D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.

Ping

Tue, Nov 20, 1:00 AM

Sun, Nov 18

Szelethus committed rC347157: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local.
[analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local
Sun, Nov 18, 4:50 AM
Szelethus committed rL347157: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local.
[analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local
Sun, Nov 18, 4:50 AM
Szelethus closed D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local.
Sun, Nov 18, 4:49 AM
Szelethus abandoned D53069: [analyzer][www] Update avaible_checks.html.

If we want to be serious about this page, it really has to be auto-generated (like clang-tidy one), but I understand that this is a larger undertaking.

Sun, Nov 18, 3:38 AM
Szelethus committed rL347153: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once.
[analyzer][UninitializedObjectChecker] Uninit regions are only reported once
Sun, Nov 18, 3:37 AM
Szelethus committed rC347153: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once.
[analyzer][UninitializedObjectChecker] Uninit regions are only reported once
Sun, Nov 18, 3:37 AM
Szelethus closed D51531: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once.
Sun, Nov 18, 3:36 AM

Fri, Nov 16

Szelethus added a comment to D51531: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once.
In D51531#1296256, @NoQ wrote:

Oh, and the reason why I think it would add a lot of complication: since only ExprEngine is avaible in the checkEndAnalysis callback, which, from what I can see, doesn't have a handly isDead method, so I'm not even sure how I'd implement it.

Mm, what do you need isDead for? Everything is dead at the end of the analysis, you need to clean up everything, otherwise you get use-after-free(?)

Oh, right. Silly me :)

Fri, Nov 16, 8:36 PM
Szelethus edited dependencies for D53280: [analyzer] Emit an error for invalid -analyzer-config inputs, added: 1; removed: 1.
Fri, Nov 16, 8:33 PM
Szelethus added a dependent revision for D53692: [analyzer] Evaluate all non-checker config options before analysis: D53280: [analyzer] Emit an error for invalid -analyzer-config inputs.
Fri, Nov 16, 8:33 PM
Szelethus removed a dependent revision for D53277: [analyzer][NFC] Collect all -analyzer-config options in a .def file: D53280: [analyzer] Emit an error for invalid -analyzer-config inputs.
Fri, Nov 16, 8:33 PM
Szelethus updated the diff for D53280: [analyzer] Emit an error for invalid -analyzer-config inputs.
  • Added the discussed compatibility flag that's off by default, but is enabled by default by the driver.
  • Now emitting errors instead of warnings.
  • For the first time, introduce errors rather then asserts for invalid input.
Fri, Nov 16, 8:32 PM
Szelethus added a comment to D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals..
In D54557#1301869, @NoQ wrote:

Nice catch, would you like to make an issue on their project or shall I?

I guess it can be an outright pull request :) I'll see if i have time for that soon, please feel free to take initiative><

I'll do the honors then. :)

Yup, there seems to be a desire to keep it around. Let's add an entry to the open projects maybe?

Mmm, what is the project here? What changes are still necessary?

Fri, Nov 16, 4:31 PM
Szelethus added a comment to D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals..
In D54557#1300653, @NoQ wrote:

I think we should either remove the non-default functionality (which wouldn't be ideal), or emphasise somewhere (open projects?) that there is still work to be done, but leaving it to be forgotten and essentially making it an extra maintenance work would be, in my optinion, the worst case scenario. Aggressive isn't Pedantic because it actually emits warnings on correct code, and it's not a simple matter of too many reports being emitted, let's also document that this is an experimental feature, not a power-user-only thing.

I only kept the option around because i was under an impression that i'm intruding into a checker that already has some happy users, probably breaking existing workflows. If this option is unnecessary, i'd be happy to remove it :)

Hmm, I'll ask around, but I'm not aware of any ongoing (or planned in the near future) work on this particular checker.

Fri, Nov 16, 3:56 PM

Thu, Nov 15

Szelethus added a comment to D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals..
In D54557#1300581, @NoQ wrote:

Whenever I compile Rtags, I get a bunch of move related warnings, I'm curious how this patch behaves on that project. I'll take a look.

Whoops, sry, i accidentally had a look because i misread your message as if you wanted me to take a look >.<

(IMPORTANT) Spoiler alert!

It finds one positive (and one duplicate of that one positive):

I believe this positive is a real bug, but we can still do better here. We find it as a use-after-move of a local variable, which is not a bug on its own, i.e. the user intended to empty and then re-use the storage and it's fine, this is not a usual kind of typo where the user uses the pre-move variable instead of the post-move variable. But the real bug here is that this local variable uses an std::string as a field under the hood, which is, well, not guaranteed to be empty after move, like all other STL objects: https://stackoverflow.com/questions/27376623/can-you-reuse-a-moved-stdstring

NOTE: As also mentioned in this stackoverflow thread, we also need to exclude smart pointers from the STL check because they don't end up in unspecified state, and see if there are other cornercases here.

OMG That is cool. :D That project was fishy. Nice catch, would you like to make an issue on their project or shall I?

Unfortunately, we don't find this bug as an STL use-after-move bug because inlining isn't happening. Why? I guess i'll leave it as an excercise ^.^ It's a combination of running out of budget and a specific feature that we have. By flipping a single -analyzer-config flag that represents that feature, we are able to find such bugs as STL bugs (when local variable bugs are disabled in the checker). We're still not able to find the original bug, most likely due to running out of budget (i didn't debug this further), but we can find it in a minimal example:

#include "rct/Rct.h"

void foo() {
  String S1;
  String S2 = std::move(S1);
  S1 += "asdfg"; // use-after-move of a std::string
}

Here's the report that we are able to obtain for this trivial code snippet, and you can look up the answer to the exercise in the collapsed run-line :)

Thanks! :) *throws confetti*

Thu, Nov 15, 6:52 PM
Szelethus committed rL347006: [analyzer] ConversionChecker: handle floating point.
[analyzer] ConversionChecker: handle floating point
Thu, Nov 15, 5:04 PM
Szelethus committed rC347006: [analyzer] ConversionChecker: handle floating point.
[analyzer] ConversionChecker: handle floating point
Thu, Nov 15, 5:04 PM
Szelethus closed D52730: [analyzer] ConversionChecker: handle floating point.
Thu, Nov 15, 5:04 PM
Szelethus added a comment to D52730: [analyzer] ConversionChecker: handle floating point.

Could someone with commit rights commit this patch (if it is acceptable)? I don't have commit rights myself.

Thu, Nov 15, 4:40 PM
Szelethus added a comment to D54438: [analyzer][WIP] Reimplement dependencies between checkers.
In D54438#1297602, @NoQ wrote:

Hmm, makes sense. Maybe static bool BlahBlahChecker::isApplicable(const LangOpts &LO) or something like that.

Btw, maybe instead of default constructor and ->setup(Args) method we could stuff all of the setup(Args)'s arguments into the one-and-only constructor for the checker?

Thu, Nov 15, 9:45 AM
Szelethus added a comment to D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals..

Alpha checkers, at least to developers, are clearly stating "Please finish me!", but a non-alpha, enabled-by-default checker with a flag that you'd never know about unless you looked at the source code (at least up until I fix these) sounds like it'll be forgotten like many other similar flags.

Thu, Nov 15, 8:00 AM
Szelethus accepted D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods..

How about std::list<T>::splice?

Thu, Nov 15, 7:53 AM
Szelethus accepted D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit..

Awesome! :D

Thu, Nov 15, 7:48 AM
Szelethus added a comment to D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals..

I think we should either remove the non-default functionality (which wouldn't be ideal), or emphasise somewhere (open projects?) that there is still work to be done, but leaving it to be forgotten and essentially making it an extra maintenance work would be, in my optinion, the worst case scenario. Aggressive isn't Pedantic because it actually emits warnings on correct code, and it's not a simple matter of too many reports being emitted, let's also document that this is an experimental feature, not a power-user-only thing.

Thu, Nov 15, 7:39 AM
Szelethus accepted D54556: [analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name..

LGTM, both the concept and the patch! I have read your followup patches up to part 4, I'll leave some thoughts there.

Thu, Nov 15, 7:15 AM

Wed, Nov 14

Szelethus added inline comments to D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded.
Wed, Nov 14, 11:17 AM
Szelethus updated the diff for D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded.
  • Added more documentation
  • Changed the nullpointer semantics as @xazax.hun pointed out
  • Realize that macro arguments themselves can be macros, handle them too.
  • Add a bunch more test. The above point actually fixed a wrong macro expansion is CALL_LAMBDA that somehow didn't get noticed. Gotta pay more attention when making the test files.
Wed, Nov 14, 11:16 AM
Szelethus added a comment to D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields.

@george.karpenkov Matching macros is a very non-trivial job, how would you feel if we shipped this patch as-is, and maybe leave a TODO about adding macro asserts down the line?

Wed, Nov 14, 10:51 AM
Szelethus added a comment to D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded.

Unfortunately, I found yet another corner case I didn't cover: if the macro arguments themselves are macros. I already fixed it, but it raises the question that what other things I may have missed? I genuinely dislike this project, and I fear that I can't test this enough to be be 100% it does it's job perfectly. Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is unnerving to put it lightly, but doing changes within Preprocessor is most definitely out of my reach.

Wed, Nov 14, 9:41 AM
Szelethus added a comment to D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry.

Unfortunately, I found yet another corner case I didn't cover: if the macro arguments themselves are macros. I already fixed it, but it raises the question that what other things I may have missed? I genuinely dislike this project, and I fear that I can't test this enough to be be 100% it does it's job perfectly. Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is unnerving to put it lightly, but doing changes within Preprocessor is most definitely out of my reach.

Wed, Nov 14, 9:40 AM

Tue, Nov 13

Szelethus added a comment to D54438: [analyzer][WIP] Reimplement dependencies between checkers.

Thanks you so much! One of the reasons why I love working on this is because the great feedback I get. I don't wanna seem annoyingly grateful, but it's been a very enjoyable experience so far.

Tue, Nov 13, 1:19 PM
Szelethus added a comment to D54466: [Analyzer] Iterator Checkers - Use the region of the topmost base class for iterators stored in a region.

Hmmm, shouldn't we add this to MemRegion's interface instead?

Tue, Nov 13, 7:00 AM · Restricted Project
Szelethus planned changes to D54438: [analyzer][WIP] Reimplement dependencies between checkers.

Here's how I'm planning to approach this issue:

  • Split MallocChecker and CStringChecker up properly, introduce MallocCheckerBase and CStringCheckerBase. I'm a little unsure about how I'll do this, but I'll share my thoughts when I come up with something clever.
  • Some checkers do not register themselves in all cases[1], which should probably be handled at a higher level, preferably in Checkers.td. Warnings could be emitted when a checker is explicitly enabled but will not be registered.
  • Once all checkers in all cases properly register themselves, refactor this patch to introduce dependencies in between the subcheckers of MallocChecker and CStringChecker. This will imply that every registry function registers one, and only one checker.
  • Move CheckerManager::registerChecker<T> out of the registry functions, rename them from register##CHECKERNAME to setup##CHECKERNAME, and supply the mutable checker object and the immutable CheckerManager. The downside is that all checkers will have to be default constructible, and their fields will have to be set up in setup##CHECKERNAME, but I see this as an acceptable tradeoff for the extra security.
    • This will make it impossible to affect other checkers. I'm a little unsure whether this is achievable considering how intertwined everything is in MallocChecker, but even if the supplied CheckerManager will have to non-const, getCurrentCheckerName and setCurrentCheckerName could be eliminated for sure.
  • Properly document why CheckerRegistry and CheckerManager are separate entities, how are the tblgen files processed, how are dependencies handled, and so on.
  • Rebase my local checker option-related branches, and celebrate. Unless another skeleton falls out of the closet. You never know I guess.
Tue, Nov 13, 6:05 AM
Szelethus updated the summary of D54438: [analyzer][WIP] Reimplement dependencies between checkers.
Tue, Nov 13, 2:36 AM
Szelethus added a comment to D54438: [analyzer][WIP] Reimplement dependencies between checkers.

Allow me to disagree again -- the reason why CheckerRegistry and the entire checker registration process is a mess is that suboptimal solutions were added instead of making the system do well on the long term. This is completely fine, I am guilty of this myself, but now that this problem has been highlighted, it's time to get things right. Some clever refactoring is long overdue.

Tue, Nov 13, 2:14 AM

Mon, Nov 12

Szelethus added a comment to D54438: [analyzer][WIP] Reimplement dependencies between checkers.

Actually, no. The main problem here is that InnerPointerChecker doesn't only want to register MallocBase, it needs to modify the checker object. In any case, either MallocChecker.cpp needs the definition of InnerPointerChecker, or vice versa. Sure, I could separate out the part that adds a new way to register dependencies, but it doesn't stop anyone from making the same mistake again, so what would be the point of that? I think it would be just a lot cleaner to split those checkers up properly, and completely replace the current system of registering checkers to stop anyone accidentally doing the same mistake again.

Mon, Nov 12, 3:55 PM
Szelethus added a comment to D54438: [analyzer][WIP] Reimplement dependencies between checkers.
In D54438#1296239, @NoQ wrote:

Mm, i don't understand. I mean, what prevents you from cutting it off even earlier and completely omitting that part of the patch? Somebody will get to this later in order to see how exactly does the separation needs to be performed.

Mon, Nov 12, 3:28 PM
Szelethus added inline comments to D53982: Output "rule" information in SARIF.
Mon, Nov 12, 2:40 PM
Szelethus added a comment to D54438: [analyzer][WIP] Reimplement dependencies between checkers.
In D54438#1296155, @NoQ wrote:

Can we reduce this patch to simply introducing the dependency item in Checkers.td and using it in, like, one place (eg., MallocChecker/CStringChecker inter-op) and discuss the rest of the stuff later before we jump into it?

Mon, Nov 12, 2:36 PM
Szelethus updated the diff for D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.

Restored the discussed note.

Mon, Nov 12, 1:12 PM
Szelethus updated the summary of D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend.
Mon, Nov 12, 12:17 PM
Szelethus updated the summary of D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend.
Mon, Nov 12, 12:16 PM
Szelethus added inline comments to D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.
Mon, Nov 12, 12:07 PM
Szelethus added a dependency for D54438: [analyzer][WIP] Reimplement dependencies between checkers: D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry.
Mon, Nov 12, 12:04 PM
Szelethus added a dependent revision for D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry: D54438: [analyzer][WIP] Reimplement dependencies between checkers.
Mon, Nov 12, 12:04 PM
Szelethus created D54438: [analyzer][WIP] Reimplement dependencies between checkers.
Mon, Nov 12, 12:03 PM
Szelethus added a dependent revision for D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp: D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend.
Mon, Nov 12, 11:40 AM
Szelethus added a dependency for D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend: D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.
Mon, Nov 12, 11:40 AM
Szelethus added a dependency for D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry: D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend.
Mon, Nov 12, 11:38 AM
Szelethus added a dependent revision for D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend: D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry.
Mon, Nov 12, 11:38 AM
Szelethus created D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry.
Mon, Nov 12, 11:38 AM
Szelethus added a comment to D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.

Also, remove diags::note_suggest_disabling_all_checkers.

Isn't that a separate revision? Given that removing existing options is often questionable, I'd much rather see this patch separated.

Mon, Nov 12, 11:29 AM
Szelethus added a comment to D54429: [analyzer] Creating standard Sphinx documentation.

What do you propose we should do with other pages on the existing website? (OpenProjects / etc.)

Mon, Nov 12, 11:29 AM · Restricted Project
Szelethus created D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend.
Mon, Nov 12, 11:22 AM
Szelethus updated the diff for D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.

Might as well make it a method.

Mon, Nov 12, 11:04 AM
Szelethus committed rC346680: [analyzer] Drastically simplify the tblgen files used for checkers.
[analyzer] Drastically simplify the tblgen files used for checkers
Mon, Nov 12, 9:52 AM
Szelethus committed rL346680: [analyzer] Drastically simplify the tblgen files used for checkers.
[analyzer] Drastically simplify the tblgen files used for checkers
Mon, Nov 12, 9:52 AM
Szelethus closed D53995: [analyzer] Drastically simplify the tblgen files used for checkers.
Mon, Nov 12, 9:52 AM
Szelethus retitled D54429: [analyzer] Creating standard Sphinx documentation from Creating standard shpinx documentation for Clang Static Analyzer to [analyzer] Creating standard shpinx documentation.
Mon, Nov 12, 8:10 AM · Restricted Project
Szelethus updated subscribers of D54429: [analyzer] Creating standard Sphinx documentation.

I'll read through this as soon as possible, but a HUGE thank you for this. This is what the Static Analyzer desperately needed for years. This will trivialize documenting, so conversations about the inner workings of the analyzer will not be buried in many year old closed revisions.

Mon, Nov 12, 8:09 AM · Restricted Project
Szelethus added a comment to D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.

Thanks for taking a look! ^-^

Mon, Nov 12, 7:19 AM
Szelethus accepted D52984: [analyzer] Checker reviewer's checklist.

LGTM! Let's continue the conversation about coding standards somewhere else.

Mon, Nov 12, 7:16 AM · Restricted Project

Sun, Nov 11

Szelethus updated the diff for D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.

Also, remove diags::note_suggest_disabling_all_checkers.

Sun, Nov 11, 2:18 PM
Szelethus added a dependency for D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp: D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local.
Sun, Nov 11, 2:17 PM
Szelethus added a dependent revision for D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local: D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.
Sun, Nov 11, 2:17 PM
Szelethus created D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.
Sun, Nov 11, 2:17 PM
Szelethus added inline comments to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.
Sun, Nov 11, 1:13 PM · Restricted Project
Szelethus added inline comments to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.
Sun, Nov 11, 12:51 PM · Restricted Project
Szelethus updated the summary of D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local.
Sun, Nov 11, 12:29 PM
Szelethus created D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local.
Sun, Nov 11, 12:26 PM

Sat, Nov 10

Szelethus added inline comments to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.
Sat, Nov 10, 12:37 PM · Restricted Project
Szelethus added inline comments to D54349: [clang-tidy] new check 'readability-redundant-preprocessor'.
Sat, Nov 10, 10:39 AM · Restricted Project
Szelethus added a comment to D52984: [analyzer] Checker reviewer's checklist.

Mainly my problem is that checker files can be large and hard to maneuver, some forward declare and document everything, some are a big bowl of spaghetti, and I think having a checklist of how it should be organized to keep uniformity and readability would be great.

Sat, Nov 10, 8:31 AM · Restricted Project
Szelethus added a comment to D52984: [analyzer] Checker reviewer's checklist.
In D52984#1294258, @NoQ wrote:

Personally, I think it's detrimental to the community for subprojects to come up with their own coding guidelines. My preference is for the static analyzer to come more in line with the rest of the project (over time, organically) in terms of style, terminology, diagnostic wording, etc. However, if the consensus is that we want a separate coding standard, I think it should be explicitly documented somewhere public and then maintained as part of the project.

I tihnk it's mostly conventions of using Analyzer-specific APIs, eg. avoid addTransition() hell - i guess we already have that, or how to register custom immutable maps, or how to implement checker dependencies or inter-checker APIs, or how much do we want to split modeling and checking into separate checkers, stuff like that.

Sat, Nov 10, 8:26 AM · Restricted Project
Szelethus added inline comments to D52984: [analyzer] Checker reviewer's checklist.
Sat, Nov 10, 7:03 AM · Restricted Project
Szelethus added a comment to D51531: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once.

Ping, @NoQ, do you insist on a global set?

Sat, Nov 10, 2:13 AM

Fri, Nov 9

Szelethus added a comment to D53995: [analyzer] Drastically simplify the tblgen files used for checkers.

I agree that it's aesthetically satisfying, but it is (1) inconvenient to write because that's the whole new syntax you need to memorize, i.e. all those <> and semicolons and (2) not cooperating when i want to look up checker name by source file (have to scan a few pages up to see the alias for the package and then jump multiple times to see what it expands to). So i'm still for ditching tblgen eventually :) But i definitely don't insist.

Fri, Nov 9, 3:26 PM
Szelethus added a comment to D53692: [analyzer] Evaluate all non-checker config options before analysis.

Thanks! This patch was the last for a while directly related to non-checker config options, I'm currently stuck on them as I unearthed countless bugs that I have to fix beforehand. (Did you know that clang unit tests do not run with check-clang-analysis?)

Fri, Nov 9, 3:05 PM
Szelethus added a comment to D53995: [analyzer] Drastically simplify the tblgen files used for checkers.

Ping

Fri, Nov 9, 10:08 AM

Wed, Nov 7

Szelethus added a comment to D52984: [analyzer] Checker reviewer's checklist.
In D52984#1269850, @NoQ wrote:
  • Warning and note messages should be clear and easy to understand, even if a bit long.
    • Messages should start with a capital letter (unlike Clang warnings!) and should not end with ..
    • Articles are usually omitted, eg. Dereference of a null pointer -> Dereference of null pointer.

I don't see this point in the list.

Wed, Nov 7, 5:45 AM · Restricted Project
Szelethus added a comment to D50211: [analyzer] Fix displayed checker name for InnerPointerChecker.

I personally find the registration process very error-prone, the current infrastructure around it makes it super easy to mess up in a way that won't be detected for months, so we probably need a better approach altogether. It shouldn't be possible to do any harm within a registry function, especially not in a way that affects other checkers.

Wed, Nov 7, 5:41 AM
Szelethus added a comment to D50211: [analyzer] Fix displayed checker name for InnerPointerChecker.

Bad news, this approach doesn't work too well, and here's why: https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/CheckerRegistry.cpp#L115

Wed, Nov 7, 5:31 AM
Szelethus added inline comments to D52730: [analyzer] ConversionChecker: handle floating point.
Wed, Nov 7, 5:18 AM

Tue, Nov 6

Szelethus added inline comments to D53692: [analyzer] Evaluate all non-checker config options before analysis.
Tue, Nov 6, 2:26 PM
Szelethus updated the diff for D53692: [analyzer] Evaluate all non-checker config options before analysis.
  • Moved initializer functions to CompilerInvocation.cpp, like every other Options-like class.
  • The fields for options are no longer Optional
  • Fixed the test files
Tue, Nov 6, 2:25 PM
Szelethus added a comment to D52984: [analyzer] Checker reviewer's checklist.

Also, context is missing :)

Tue, Nov 6, 4:08 AM · Restricted Project
Szelethus added a comment to D52984: [analyzer] Checker reviewer's checklist.

We probably should also add an entry about some code conventions we use here, for example, the use of auto was debated recently when used with SVal::getAs, maybe something like this:

  • As an LLVM subproject, the code in the Static Analyzer should too follow the https://llvm.org/docs/CodingStandards.html, but we do have some further restrictions/additions to that:
    • Prefer the usage of auto when using SVal::getAs, and const auto * when using MemRegion::getAs. The coding standard states that "Use auto if and only if it makes the code more readable or easier to maintain.", and even though SVal::getAs<T> returns with llvm::Optional<T>, which is not completely trivial, SVal is one of the most heavily used types in the Static Analyzer, and this convention makes the code significantly more readable.
    • Use llvm::SmallString and llvm::raw_svector_ostream to construct report messages. The coding standard explicitly doesn't forbid the use of the <sstream> library, but we do.
    • Do not forward declare or define static functions inside an anonymous namespace in checker files.
    • Document checkers on top of the file, rather then with comments before the checker class.
    • Checker files usually contain the checker class, the registration functions, and of course the actual implementation (usually several more functions and data structures) of the checker logic. To make the code more readable, keep the checker class on top of the file, forward declare all data structures and implementation functions below that, and put the registration function on the very bottom. The actual logic should be in between. For example:
//===----- MyChecker.cpp -----------------------------------------*- C++ -*-==//
//
//                     The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
//
// MyChecker is a tool to show an example how a checker file should be
// organized.
//
//===----------------------------------------------------------------------===//
Tue, Nov 6, 3:59 AM · Restricted Project
Szelethus added inline comments to D53974: [clang-tidy] new check: bugprone-too-small-loop-variable.
Tue, Nov 6, 1:17 AM · Restricted Project

Mon, Nov 5

Szelethus added a comment to D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols..

I have no other objections, looks great!

Mon, Nov 5, 1:59 PM
Szelethus added inline comments to D54013: [analyzer] NFC: MallocChecker: Avoid redundant transitions..
Mon, Nov 5, 1:22 PM
Szelethus added a comment to D52790: [analyzer][PlistMacroExpansion] New flag to convert macro expansions to events.
In D52790#1285039, @NoQ wrote:

Looks great, let's land?

I'll probably land it after part 5, in order to ease on rebasing.

Not sure if i already asked - am i understanding correctly that this is a "poor-man's" support for macro expansions for tools that read plists but do not support those new plist macro dictionaries yet?

Correct, same as notes-as-events.

I wonder how expansions of huge macros will look as events.

Quite funny, actually :D Think about assert, it's quite a mouthful. I'll post screenshots when I have more time.

Mon, Nov 5, 11:25 AM
Szelethus added inline comments to D54013: [analyzer] NFC: MallocChecker: Avoid redundant transitions..
Mon, Nov 5, 10:59 AM