xazax.hun (Gábor Horváth)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 17 2012, 3:16 AM (265 w, 6 d)

Recent Activity

Fri, Oct 20

xazax.hun added a comment to D33537: [clang-tidy] Exception Escape Checker.

I agree that we should not spend too much effort on making warnings from the compiler and tidy disjunct.

Fri, Oct 20, 7:18 AM · Restricted Project
xazax.hun added inline comments to D39121: [clang-tidy] Misplaced Operator in Strlen in Alloc.
Fri, Oct 20, 7:11 AM · Restricted Project
xazax.hun added inline comments to D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true.
Fri, Oct 20, 4:36 AM

Thu, Oct 19

xazax.hun added inline comments to D38845: [ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr.
Thu, Oct 19, 5:33 AM
xazax.hun updated subscribers of D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values.

I think this change is very useful but it is also important to get these changes right.
I think one of the main reason you did not get review comments yet is that it is not easy to verify that these changes are sound.

Thu, Oct 19, 4:49 AM
xazax.hun added a comment to D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true.

LGTM!

Thu, Oct 19, 3:33 AM
xazax.hun added a comment to D39049: [analyzer] Fix wrong calculation of offset in ArrayBoundsV2.

I checked what happens:

Thu, Oct 19, 3:27 AM
xazax.hun added a comment to D38171: Implement clang-tidy check aliases..

One problem to think about when we add all clang-diagnostic as "first or second" class citizen, checkes=* might now enable all the warnings which make no sense and might be surprising to the users. What do you think?

Thu, Oct 19, 2:50 AM
xazax.hun added inline comments to D38692: [ASTImporter] Support importing FunctionTemplateDecl and CXXDependentScopeMemberExpr.
Thu, Oct 19, 2:50 AM
xazax.hun added inline comments to D38694: [ASTImporter] Support importing CXXUnresolvedConstructExpr and UnresolvedLookupExpr.
Thu, Oct 19, 2:44 AM
xazax.hun added inline comments to D38843: [ASTImporter] Support importing CXXPseudoDestructorExpr.
Thu, Oct 19, 2:37 AM
xazax.hun added inline comments to D38845: [ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr.
Thu, Oct 19, 2:33 AM
xazax.hun added inline comments to D38921: [analyzer] LoopUnrolling: update the matched assignment operators.
Thu, Oct 19, 2:26 AM

Wed, Oct 18

xazax.hun accepted D39048: Dump signed integers in SymIntExpr and IntSymExpr correctly.

LGTM!

Wed, Oct 18, 5:43 AM
xazax.hun added a reviewer for D38842: [CrossTU] Fix handling of Cross Translation Unit directory path: szepet.
Wed, Oct 18, 4:57 AM
xazax.hun accepted D38922: [analyzer] LoopUnrolling: check the bitwidth of the used numbers (pr34943).

LGTM!

Wed, Oct 18, 4:53 AM
xazax.hun updated the diff for D30691: [analyzer] Support for naive cross translational unit analysis.
  • Update the scan-build part to work correctly with the accepted version of libCrossTU
Wed, Oct 18, 4:22 AM

Mon, Oct 16

xazax.hun added inline comments to D38943: [ASTImporter] import SubStmt of CaseStmt.
Mon, Oct 16, 8:16 AM
xazax.hun added inline comments to D37437: [analyzer] Fix some checker's output plist not containing the checker name.
Mon, Oct 16, 5:40 AM
xazax.hun updated the diff for D37437: [analyzer] Fix some checker's output plist not containing the checker name.
  • Address review comments.
Mon, Oct 16, 5:40 AM
xazax.hun accepted D38943: [ASTImporter] import SubStmt of CaseStmt.

LGTM with a nit.

Mon, Oct 16, 4:22 AM
xazax.hun added a reviewer for D38943: [ASTImporter] import SubStmt of CaseStmt: szepet.
Mon, Oct 16, 2:12 AM

Fri, Oct 13

xazax.hun added a comment to D38688: [clang-tidy] Misc redundant expressions checker updated for macros.

Thanks for working on this, these additions look very useful to me.

Fri, Oct 13, 4:56 AM · Restricted Project

Thu, Oct 12

xazax.hun updated the diff for D38728: [analyzer] Use the signature of the primary template for issue hash calculation.
  • Rebase based on the dependent revision and minor cleanups
Thu, Oct 12, 7:36 AM
xazax.hun added a dependent revision for D38844: [analyzer] Make issue hash related tests more concise: D38728: [analyzer] Use the signature of the primary template for issue hash calculation.
Thu, Oct 12, 7:16 AM
xazax.hun added a dependency for D38728: [analyzer] Use the signature of the primary template for issue hash calculation: D38844: [analyzer] Make issue hash related tests more concise.
Thu, Oct 12, 7:16 AM
xazax.hun updated the diff for D38844: [analyzer] Make issue hash related tests more concise.
  • Update the name of the debug function according to review comments
Thu, Oct 12, 5:52 AM
xazax.hun created D38844: [analyzer] Make issue hash related tests more concise.
Thu, Oct 12, 5:45 AM
xazax.hun created D38842: [CrossTU] Fix handling of Cross Translation Unit directory path.
Thu, Oct 12, 5:05 AM
xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.
  • Unittests now creates temporary files at the correct location
  • Temporary files are also cleaned up when the process is terminated
  • Absolute paths are handled correctly by the library

I think there is an issue with this change.

First, the function map generation writes absolute paths to the map file. Now when the parseCrossTUIndex function parses it, it will no longer prepend the paths with the CTUDir and therefore expect the precompiled AST files at the exact path of the source file. This seems like a bad requirement, because the user would have to overwrite his source files.

If I understand your intention correctly, a fix would be to replace the is_absolute check with a check for CTUDir == "" in the parseCrossTUIndex function.

Thu, Oct 12, 3:12 AM
xazax.hun added a comment to D38728: [analyzer] Use the signature of the primary template for issue hash calculation.
In D38728#895669, @NoQ wrote:

I think it would be great to split them into two different patches, to be able to easily see how the change in the hashing affects the tests (and maybe revert easily if something goes wrong).

Thu, Oct 12, 1:59 AM
xazax.hun updated the diff for D38728: [analyzer] Use the signature of the primary template for issue hash calculation.
  • Added some comments to the tests
Thu, Oct 12, 1:01 AM

Wed, Oct 11

xazax.hun added inline comments to D38728: [analyzer] Use the signature of the primary template for issue hash calculation.
Wed, Oct 11, 4:45 AM
xazax.hun updated the diff for D38728: [analyzer] Use the signature of the primary template for issue hash calculation.
  • Added more test cases
  • Made the tests more concise
Wed, Oct 11, 4:45 AM

Tue, Oct 10

xazax.hun created D38728: [analyzer] Use the signature of the primary template for issue hash calculation.
Tue, Oct 10, 6:43 AM
xazax.hun added inline comments to D38673: [analyzer] MisusedMovedObjectChecker: Fix false positive on state-resetting, handling method calls on base-class sub-objects.
Tue, Oct 10, 6:22 AM
xazax.hun added a comment to D38171: Implement clang-tidy check aliases..

Let's also summarize what do we have now and what do we want.

Tue, Oct 10, 4:35 AM

Mon, Oct 9

xazax.hun added a comment to D38702: [Analyzer] Do not segfault on unexpected call_once implementation.

Did you link the correct bug in the description? The one you linked was closed long ago.

Mon, Oct 9, 1:05 PM
xazax.hun accepted D38673: [analyzer] MisusedMovedObjectChecker: Fix false positive on state-resetting, handling method calls on base-class sub-objects.

LGTM! I only found a nit.

Mon, Oct 9, 6:39 AM
xazax.hun accepted D38674: [analyzer] MisusedMovedObjectChecker: More precise warning message.

LGTM!

Mon, Oct 9, 6:29 AM
xazax.hun accepted D31538: [analyzer] MisusedMovedObjectChecker: Fix a false positive on state-resetting a base-class sub-object..

I think there was only one comment but that is already addressed in a dependent revision. So I think this one is good as is.

Mon, Oct 9, 6:14 AM
xazax.hun accepted D31541: [analyzer] MisusedMovedObjectChecker: Add a printState() method..

LGTM!

Mon, Oct 9, 6:13 AM
xazax.hun added a comment to D38675: [analyzer] MisusedMovedObjectChecker: Moving the checker out of alpha state.

However, the checker seems to work with a low false positive rate. (<15 on the LLVM, 6 effectively different)

This does not sound like a low false positive rate to me. Could you describe what the false positives are? Is it possible to fix them?

Mon, Oct 9, 6:13 AM

Mon, Sep 25

xazax.hun added a comment to D38171: Implement clang-tidy check aliases..

A mail [0] posted by @JonasToth is about the similar problem, i think we can continue there.

Mon, Sep 25, 6:17 AM
xazax.hun added a comment to D38171: Implement clang-tidy check aliases..

András, that's definitely an interesting idea. However, it might be interesting to explore a more principled approach:

  1. Make clang-diagnostic-* checks first-class citizens and take full control of all diagnostics, i.e. disable all Clang diagnostics by default, and enable the ones that correspond to the enabled clang-diagnostic checks.
Mon, Sep 25, 6:03 AM
xazax.hun added a comment to D38171: Implement clang-tidy check aliases..

@leanil : Could you add a test case where the warnings are explicitly disabled in the compilation command and also one where only the aliased warning is explicitly disabled?

Mon, Sep 25, 5:51 AM
xazax.hun added a comment to D30691: [analyzer] Support for naive cross translational unit analysis.

For my purposes I replaced the return statement of the compareCrossTUSourceLocs function with:

return XL.getFileID() < YL.getFileID();

A more correct fix would create only one unique diagnostic for both cases.

Mon, Sep 25, 5:03 AM
xazax.hun added a comment to D30691: [analyzer] Support for naive cross translational unit analysis.

If either of the FullSourceLocs is a MacroID, the call SM.getFileEntryForID(XL.getFileID()) will return a null pointer. The null pointer will crash the program when attempting to call ->getName() on it.

Mon, Sep 25, 5:03 AM
xazax.hun updated the diff for D30691: [analyzer] Support for naive cross translational unit analysis.
  • Fixed an issue with source locations
  • Updated to latest trunk
Mon, Sep 25, 5:03 AM

Sep 22 2017

xazax.hun added a reviewer for D38171: Implement clang-tidy check aliases.: aaron.ballman.
Sep 22 2017, 4:02 AM

Sep 21 2017

xazax.hun updated the diff for D34512: Add preliminary Cross Translation Unit support library.
  • Unittests now creates temporary files at the correct location
  • Temporary files are also cleaned up when the process is terminated
  • Absolute paths are handled correctly by the library
Sep 21 2017, 8:30 AM
xazax.hun added inline comments to D37812: [analyzer] PthreadLock: Escape the pointers..
Sep 21 2017, 1:44 AM
xazax.hun accepted D37963: [analyzer] PthreadLock: Don't track dead regions..

Maybe it worth a comment why we do not want to clean up LockSet? Otherwise LGTM!

Sep 21 2017, 1:37 AM
xazax.hun added a comment to D37437: [analyzer] Fix some checker's output plist not containing the checker name.

Ping.

Sep 21 2017, 1:29 AM
xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.

I'm OK with this going into the repo a is (although it is light on tests!), as long as we have an agreement that you'll be OK with iteration on both the interface and the implementation to handle real-world projects.

Sep 21 2017, 1:10 AM

Sep 18 2017

xazax.hun created D37978: [analyzer] Fix an assertion fail in VirtualCallChecker.
Sep 18 2017, 8:09 AM

Sep 15 2017

xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.

Any further reviews?
What are the criteria to accept this patch? Who or how many people should accept this?

Sep 15 2017, 4:47 AM
xazax.hun added a comment to D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written.

I have some comments and questions but maybe you do not want to address those until Devin, NoQ, or Anna approved the general directions.

Sep 15 2017, 4:25 AM
xazax.hun added a comment to D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written.

Out of curiosity, does the false positive disappear after making the static variables const?

Sep 15 2017, 3:16 AM

Sep 7 2017

xazax.hun added inline comments to D34512: Add preliminary Cross Translation Unit support library.
Sep 7 2017, 2:18 AM
xazax.hun updated the diff for D34512: Add preliminary Cross Translation Unit support library.
  • Address review comments.
Sep 7 2017, 2:18 AM
xazax.hun added a comment to D32981: [ASTImporter] Improve handling of incomplete types .

Is this blocked on something?

Sep 7 2017, 1:08 AM

Sep 6 2017

xazax.hun accepted D37478: [analyzer] Implement pointer arithmetic on constants.

One nit, otherwise LGTM! Thanks for fixing this!

Sep 6 2017, 4:08 AM

Sep 5 2017

xazax.hun added a comment to D33729: [analyzer] lock_guard and unique_lock extension for BlockInCriticalSection Static Analyzer checker.

Ping... While extending isCalled is not a must, this should not crash on the sample ObjC code.

Sep 5 2017, 5:52 AM
xazax.hun created D37470: [analyzer] Handle ObjC messages conservatively in CallDescription.
Sep 5 2017, 5:51 AM
xazax.hun added a comment to D37437: [analyzer] Fix some checker's output plist not containing the checker name.
In D37437#860311, @NoQ wrote:

Cool. Thanks!

In the future probably it would be better to alter the signature of the checkers' constructor to set the name in the constructor so it is possible to create the BugType eagerly.

Still, should we add an assertion so that we could be sure that every bug type contains a checker name?

Sep 5 2017, 5:15 AM
xazax.hun updated the diff for D37437: [analyzer] Fix some checker's output plist not containing the checker name.
  • Fix more checkers
  • Minor style fixes along the way
  • Added an assert to prevent such errors in the future
Sep 5 2017, 5:14 AM

Sep 4 2017

xazax.hun updated the summary of D37437: [analyzer] Fix some checker's output plist not containing the checker name.
Sep 4 2017, 7:16 AM
xazax.hun created D37437: [analyzer] Fix some checker's output plist not containing the checker name.
Sep 4 2017, 7:15 AM
xazax.hun updated the diff for D34512: Add preliminary Cross Translation Unit support library.
  • Make the API capable of using custom lookup strategies for CTU
  • Fix review comments
Sep 4 2017, 4:57 AM
xazax.hun added inline comments to D34512: Add preliminary Cross Translation Unit support library.
Sep 4 2017, 4:57 AM

Aug 30 2017

xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.

In either case, the important scenario I think we should support is choosing at a call site to a C function the most likely definition of the called function, based on number and type of parameters, from multiple possible definitions in other translation units. If the API is rich enough to support this then I think that is a good indication it will be useful for other scenarios as well.

Note that the lookup is already based on USR which is similar to mangled names in a sense that it contains information about the function parameters. So the only way to get multiple candidates from the lookup is having multiple function definitions with the same signature.

I just want to clarify that C function USRs do not contain type information, although C++ USRs do.

Aug 30 2017, 10:01 AM
xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.

In either case, the important scenario I think we should support is choosing at a call site to a C function the most likely definition of the called function, based on number and type of parameters, from multiple possible definitions in other translation units. If the API is rich enough to support this then I think that is a good indication it will be useful for other scenarios as well.

Aug 30 2017, 1:47 AM
xazax.hun updated the diff for D34512: Add preliminary Cross Translation Unit support library.
  • Added unit test to ensure the produced index format can be parsed.
  • Added further diagnostics.
Aug 30 2017, 1:47 AM

Aug 29 2017

xazax.hun updated the diff for D34512: Add preliminary Cross Translation Unit support library.
  • Updates according to review comments.
Aug 29 2017, 7:03 AM
xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.

Except for some drive-by nits, this is a high-level review.

I have some high-level questions about the design of the library:

  1. How do you intend to handle the case when there are multiple function definitions with the same USR? Whose responsibility it is to deal with this: the client (for example, the static analyzer) or the index/database (libCrossTU)? This seems to me to be a fundamental part of the design for this feature that is missing. If you expect the client to handle this scenario (for example, to pick a definition) I would expect the library API to return more than a single potential result for a query. If you expect the the library to pick a definition then at a minimum you should document how this will be done. If the library will treat this as an error then you should communicate this to the client so it can attempt to recover from it.
Aug 29 2017, 2:01 AM

Aug 28 2017

xazax.hun added a comment to D37023: [analyzer] Fix bugreporter::getDerefExpr() again..
In D37023#853941, @NoQ wrote:

Thank you for the review!

Though it looks like some of the cases covered in the code do not have corresponding tests (e.g.: the parenexprs).

These are covered by tests in inline-defensive-checks.c:150,156,169,179 (old code had IgnoreParenCasts). This function is actually used a lot (even more so since D32291), and seems fairly well-tested, so i felt relatively safe when refactoring it.

Aug 28 2017, 6:14 AM
xazax.hun accepted D37025: [analyzer] Support more pointer arithmetic in bugreporter::getDerefExpr()..

LGTM!

Aug 28 2017, 3:48 AM
xazax.hun accepted D37023: [analyzer] Fix bugreporter::getDerefExpr() again..

Generally, it looks good to me. Though it looks like some of the cases covered in the code do not have corresponding tests (e.g.: the parenexprs).
I think this approach is good in a sense there should be such a cast at the interesting places. But I wonder if there could be some cases where we have such cast earlier than expected.

Aug 28 2017, 3:48 AM

Aug 18 2017

xazax.hun updated subscribers of D34275: [analyzer] Re-implemente current virtual calls checker in a path-sensitive way.

@NoQ , @dcoughlin could either of you review this patch as well? The end of GSoC is closing and it would be awesome to be able to commit this before it ends. :)

Aug 18 2017, 11:37 PM

Aug 15 2017

xazax.hun added a comment to D36737: [analyzer] Store design discussions in docs/analyzer for future use..

Good idea!

Aug 15 2017, 6:53 AM
xazax.hun added inline comments to D36670: misc-misplaced-widening-cast: fix assertion.
Aug 15 2017, 3:45 AM · Restricted Project
xazax.hun accepted D36670: misc-misplaced-widening-cast: fix assertion.

LGTM!

Aug 15 2017, 3:43 AM · Restricted Project

Aug 10 2017

xazax.hun added a comment to D34508: [Analyzer] Bug Reporter Visitor to Display Values of Variables - PRELIMINARY!.
In D34508#791048, @NoQ wrote:

Currently, we already highlight the last assignments for the "interesting" variables, which is implemented through, for example, bugreporter::trackNullOrUndefValue() - see how various checkers use it. This is, of course, far from perfect as well, because it's very hard to figure out which variables are of interest.

Aug 10 2017, 8:04 AM
xazax.hun updated the diff for D34512: Add preliminary Cross Translation Unit support library.
  • Address review comments
Aug 10 2017, 4:54 AM
xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.

Apart from those in the in-line comments, I have a question: how safe is this library to Release builds? I know this is only a submodule dependency for the "real deal" in D30691, but I have seen some asserts that "imported function should already have a body" and such.

Will the static analyzer handle these errors gracefully and fall back to "function is unknown, let's throw my presumptions out of the window" or will it bail away? I'm explicitly thinking of the assert-lacking Release build.

Aug 10 2017, 2:26 AM
xazax.hun added inline comments to D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker.
Aug 10 2017, 1:58 AM

Aug 8 2017

xazax.hun added a comment to D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values.

Can't you reuse somehow some machinery already available to evaluate the arithmetic operators? Those should already handle most of your TODOs and overflows.

Aug 8 2017, 8:38 AM
xazax.hun added inline comments to D35068: [analyzer] Detect usages of unsafe I/O functions.
Aug 8 2017, 1:32 AM
xazax.hun added a comment to D35068: [analyzer] Detect usages of unsafe I/O functions.
In D35068#811437, @NoQ wrote:

It'd look good in clang-tidy (especially if extended to provide fixits), but if Daniel is interested in having this feature in the analyzer (and picked by clang-tidy from there), i wouldn't mind.

I wonder how noisy this check is - did you test it on large codebases? Because these functions are popular, and in many cases it'd be fine to use insecure functions, i wonder if it's worth it to have this check on by default. Like, if it's relatively quiet - it's fine, but if it'd constitute 90% of the analyzer's warnings on popular projects, that'd probably not be fine.

Aug 8 2017, 1:30 AM
xazax.hun accepted D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width.

It looks good to me but let's wait for Anna, NoQ, or Devin for the final word.

Aug 8 2017, 1:25 AM

Aug 3 2017

xazax.hun updated the diff for D34512: Add preliminary Cross Translation Unit support library.
  • Address further review comments.
Aug 3 2017, 7:35 AM
xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.

Organizationally, this seems fine. Carry on :)

Aug 3 2017, 7:17 AM
xazax.hun updated the diff for D34512: Add preliminary Cross Translation Unit support library.
  • Addressed review comments.
Aug 3 2017, 7:17 AM

Aug 1 2017

xazax.hun added a comment to D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker.

Even though it is not undefined behavior in C, it can still cause surprising behavior for the users. I think maybe putting it into the optin package instead of cplusplus is better. What do you think?

Aug 1 2017, 8:17 AM
xazax.hun added a comment to D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker.

Even though it is not undefined behavior in C, it can still cause surprising behavior for the users. I think maybe putting it into the optin package instead of cplusplus is better. What do you think?

Aug 1 2017, 8:16 AM
xazax.hun added inline comments to D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width.
Aug 1 2017, 6:39 AM
xazax.hun accepted D36141: [StaticAnalyzer] Fix FP in UnreachableCodeChecker.

LGTM!

Aug 1 2017, 5:55 AM

Jul 31 2017

xazax.hun added inline comments to D33722: [clang-tidy] Add checker for undelegated copy of base classes.
Jul 31 2017, 6:54 AM · Restricted Project
xazax.hun added a comment to D34512: Add preliminary Cross Translation Unit support library.

Yep, I want Richard's approval for the name. I think he already expressed a pro-pulling-out stance.

Jul 31 2017, 3:19 AM