Page MenuHomePhabricator

martong (Gabor Marton)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 10 2017, 8:01 AM (182 w, 4 d)

Recent Activity

Thu, Apr 1

martong added inline comments to D99714: [clang][Analyzer] Handle flexible arrays better in ArrayBoundV2 checker..
Thu, Apr 1, 6:31 AM · Restricted Project
martong added a comment to D99714: [clang][Analyzer] Handle flexible arrays better in ArrayBoundV2 checker..

It works not reliable for all data types. If char is used instead of int (in the test), the allocated size may be larger than the intended size of the array, probably because memory alignment adjustments. In the following case it is possible to index "past the end" of the array for some first indices (until 12?).

struct S {
  int n;
  char x;
  char s[];
};
Thu, Apr 1, 6:29 AM · Restricted Project
martong added a comment to D99714: [clang][Analyzer] Handle flexible arrays better in ArrayBoundV2 checker..

The tests are really promising! :)

Thu, Apr 1, 4:22 AM · Restricted Project
martong updated subscribers of D99714: [clang][Analyzer] Handle flexible arrays better in ArrayBoundV2 checker..
Thu, Apr 1, 4:06 AM · Restricted Project
martong accepted D99659: [analyzer][taint] Extent of heap regions should get taint sometimes.

I like it, looks good to me!

Thu, Apr 1, 3:00 AM · Restricted Project
martong accepted D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions.

LG!

Thu, Apr 1, 2:54 AM · Restricted Project

Wed, Mar 31

martong added inline comments to D99260: [analyzer] Fix false positives in inner pointer checker (PR49628).
Wed, Mar 31, 7:47 AM · Restricted Project
martong added a comment to D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions.

Ah, I see you need nonloc::SymbolVal in your next patch, and getDynamicSizeWithOffset returns an Unknown if the extend is symbolic.

Wed, Mar 31, 7:31 AM · Restricted Project
martong added inline comments to D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions.
Wed, Mar 31, 7:17 AM · Restricted Project
martong added inline comments to D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions.
Wed, Mar 31, 7:14 AM · Restricted Project
martong accepted D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h..

I went through the change and it looks good, seems like this is indeed a copy paste error from line 132.
I checked the related conversation, and thanks for all the effort spent with the test.

Wed, Mar 31, 7:06 AM · Restricted Project

Tue, Mar 30

martong accepted D99576: [ASTImporter][NFC] Improve test coverage.

LGTM! Thanks!

Tue, Mar 30, 8:14 AM · Restricted Project
martong added inline comments to D99262: [analyzer] Fix dead store checker false positive.
Tue, Mar 30, 7:05 AM · Restricted Project
martong added inline comments to D99260: [analyzer] Fix false positives in inner pointer checker (PR49628).
Tue, Mar 30, 6:47 AM · Restricted Project
martong committed rGefa7df1682c2: [Analyzer] Track RValue expressions (authored by martong).
[Analyzer] Track RValue expressions
Tue, Mar 30, 5:57 AM
martong closed D99344: [Analyzer] Track RValue expressions.
Tue, Mar 30, 5:57 AM · Restricted Project
martong added inline comments to D99344: [Analyzer] Track RValue expressions.
Tue, Mar 30, 3:12 AM · Restricted Project
martong updated the diff for D99344: [Analyzer] Track RValue expressions.
  • Update comment
Tue, Mar 30, 3:12 AM · Restricted Project
martong added a comment to D99421: [ASTImporter] Import member specialization/instantiation of enum decls.

Thanks for the review guys! :)

Tue, Mar 30, 2:58 AM · Restricted Project
martong committed rG98f6cbd68eba: [ASTImporter] Import member specialization/instantiation of enum decls (authored by martong).
[ASTImporter] Import member specialization/instantiation of enum decls
Tue, Mar 30, 2:58 AM
martong closed D99421: [ASTImporter] Import member specialization/instantiation of enum decls.
Tue, Mar 30, 2:58 AM · Restricted Project
martong updated the diff for D99421: [ASTImporter] Import member specialization/instantiation of enum decls.
  • Add check for specialization kind
Tue, Mar 30, 2:20 AM · Restricted Project

Mon, Mar 29

martong added inline comments to D99421: [ASTImporter] Import member specialization/instantiation of enum decls.
Mon, Mar 29, 7:04 AM · Restricted Project
martong updated the diff for D99421: [ASTImporter] Import member specialization/instantiation of enum decls.
  • Fix typo
Mon, Mar 29, 7:04 AM · Restricted Project
martong added inline comments to D99344: [Analyzer] Track RValue expressions.
Mon, Mar 29, 6:43 AM · Restricted Project
martong updated the diff for D99344: [Analyzer] Track RValue expressions.
  • Remove handling of assignment
Mon, Mar 29, 6:40 AM · Restricted Project
martong added a comment to D99348: [ASTImporter] Add ability to convert CXXRecordDecls to RecordDecls when importing to C language TUs.

So something like an ASTConverter that just sits on top of the ASTImporter? In theory that class could implement the ImportImpl callback and do the custom imports it needs to do on its own

I am seeing some difficulties with this approach b/c this would require to replace an existing node in the AST with a new one in some cases. And that might not be always possible without a full retraverse of the AST. Perhaps we could come up with some simplified visitation or could reuse some data from ASTImporter::ImportedDecls.

Mon, Mar 29, 2:26 AM · Restricted Project
martong added a comment to D99348: [ASTImporter] Add ability to convert CXXRecordDecls to RecordDecls when importing to C language TUs.

Hi Raphael,

Mon, Mar 29, 2:19 AM · Restricted Project

Fri, Mar 26

martong updated the diff for D99421: [ASTImporter] Import member specialization/instantiation of enum decls.
  • Rename test case name to something meaningful
Fri, Mar 26, 9:31 AM · Restricted Project
martong requested review of D99421: [ASTImporter] Import member specialization/instantiation of enum decls.
Fri, Mar 26, 9:29 AM · Restricted Project
martong added a comment to D99344: [Analyzer] Track RValue expressions.

Guys, thanks for the review! :)

Fri, Mar 26, 1:40 AM · Restricted Project
martong updated the diff for D99344: [Analyzer] Track RValue expressions.
  • Use isZeroConstant()
  • Make trackRValueExpression private (static function)
  • Fix clang-tidy report: auto *BO ---> const auto *BO
Fri, Mar 26, 1:38 AM · Restricted Project

Thu, Mar 25

martong added inline comments to D99344: [Analyzer] Track RValue expressions.
Thu, Mar 25, 10:51 AM · Restricted Project
martong updated the diff for D99344: [Analyzer] Track RValue expressions.
  • Assert in the callee and check in the caller
  • Fix typo in comment
Thu, Mar 25, 10:47 AM · Restricted Project
martong updated the diff for D99344: [Analyzer] Track RValue expressions.
  • Rebase
Thu, Mar 25, 10:46 AM · Restricted Project
martong committed rG015c39882ebc: [Analyzer] Infer 0 value when the divisible is 0 (bug fix) (authored by martong).
[Analyzer] Infer 0 value when the divisible is 0 (bug fix)
Thu, Mar 25, 10:25 AM
martong closed D99343: [Analyzer] Infer 0 value when the divisible is 0 (bug fix).
Thu, Mar 25, 10:25 AM · Restricted Project
martong added a comment to D99343: [Analyzer] Infer 0 value when the divisible is 0 (bug fix).

Thanks for the review!

Thu, Mar 25, 10:25 AM · Restricted Project
martong added inline comments to D99343: [Analyzer] Infer 0 value when the divisible is 0 (bug fix).
Thu, Mar 25, 8:12 AM · Restricted Project
martong requested review of D99344: [Analyzer] Track RValue expressions.
Thu, Mar 25, 8:08 AM · Restricted Project
martong requested review of D99343: [Analyzer] Infer 0 value when the divisible is 0 (bug fix).
Thu, Mar 25, 8:06 AM · Restricted Project

Wed, Mar 24

martong committed rGf8a850ccf452: [Analyzer][NFC] Fix typos in comments (authored by martong).
[Analyzer][NFC] Fix typos in comments
Wed, Mar 24, 3:46 AM

Tue, Mar 23

martong accepted D99162: [ASTImporter] Split out Objective-C related unit tests.

Sounds good to me!

Tue, Mar 23, 5:30 AM · Restricted Project

Mon, Mar 22

martong accepted D99062: [clang][ASTImporter] Import "CapturedVLAType" in FieldDecl..

LGTM! Thanks!

Mon, Mar 22, 3:34 AM · Restricted Project
martong accepted D98951: [clang][ASTImporter] Add import API for 'const Type *' (NFC)..

LG! Thanks!

Mon, Mar 22, 3:28 AM · Restricted Project

Mar 11 2021

martong added reviewers for D95244: [clang][AST] Handle overload callee type in CallExpr::getCallReturnType.: aaron.ballman, riccibruno, dblaikie.

Hi @aaron.ballman @riccibruno @dblaikie, I am adding you as a reviewer because it seems you have great experience with Clang's AST.
Could you please take a look on this patch? The problem is that we currently do not handle a CallExpr's return type properly in case of a dependent context. This seems to be a flaw in the implementation of CallExpr.

Mar 11 2021, 5:07 AM · Restricted Project

Mar 9 2021

martong accepted D98244: [analyzer] Fix StdLibraryFunctionsChecker performance issue.

Good catch! Thank you!

Mar 9 2021, 9:09 AM · Restricted Project
martong added inline comments to D95244: [clang][AST] Handle overload callee type in CallExpr::getCallReturnType..
Mar 9 2021, 2:34 AM · Restricted Project
martong added a comment to D95244: [clang][AST] Handle overload callee type in CallExpr::getCallReturnType..

This causes a crash if the function is called in such case.

Mar 9 2021, 2:29 AM · Restricted Project

Mar 8 2021

martong accepted D96586: [analyzer][CTU][NFC] Add an extra regression test.

Thanks for the test!

Mar 8 2021, 5:27 AM · Restricted Project

Mar 4 2021

martong added a comment to D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member.

Thanks for the review!

Mar 4 2021, 6:11 AM · Restricted Project
martong committed rG2e90fc2c407b: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member (authored by martong).
[AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member
Mar 4 2021, 6:11 AM
martong closed D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member.
Mar 4 2021, 6:10 AM · Restricted Project
martong updated the diff for D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member.

Add a test case which fails if lit --vg is used.

Mar 4 2021, 6:07 AM · Restricted Project

Mar 3 2021

martong added a comment to D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member.

We can (and do) run LIT tests under valgrind, so if you have a test that triggers this valgrind error then that seems like the way to go. I don't think the test has to deterministically fail outside valgrind to be a valid test.

Mar 3 2021, 10:15 AM · Restricted Project
martong added a comment to D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member.

This looks reasonable to me (good catch!), but is there a way for us to add a regression test for it?

I'm not sure if it's possible to write a test for deterministically demonstrating the bug - which is a non-deterministic crash.
So, even if we would have a test case, that would not catch the regression deterministically.
We could include the minimal reproducer for CTU analysis - the way we observed and tracked down this crash.

AFAIK, it did not reproduce with memory sanitizers.

Mar 3 2021, 10:11 AM · Restricted Project
martong added a comment to D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member.

Mar 3 2021, 4:52 AM · Restricted Project
martong requested review of D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member.
Mar 3 2021, 4:50 AM · Restricted Project

Feb 1 2021

martong accepted D93223: [analyzer] Create MacroExpansionContext member in AnalysisConsumer.

Still looks good.
(The parent is an NFC and only this patch changes the behavior (and only if the cmdline flag is there), right? So, I'd expect the big impact from this patch.)

Feb 1 2021, 6:15 AM · Restricted Project
martong accepted D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.

Still looks good to me! Thanks for handling the pragma cases!

Feb 1 2021, 6:09 AM · Restricted Project

Jan 27 2021

martong added reviewers for D95244: [clang][AST] Handle overload callee type in CallExpr::getCallReturnType.: rsmith, majnemer.

Adding reviewers.
@rsmith as code owner,
@majnemer based on git blame of CallExpr::getCallReturnType.

Jan 27 2021, 1:05 AM · Restricted Project

Jan 25 2021

martong added a comment to D95244: [clang][AST] Handle overload callee type in CallExpr::getCallReturnType..

Hi Balázs,

Jan 25 2021, 8:25 AM · Restricted Project

Jan 21 2021

martong added a comment to D69726: [analyzer] DynamicSize: Store the dynamic size.

I was referred to this patch from https://reviews.llvm.org/D86743. I pulled this patch under review, brought it up to date and pushed to github at https://github.com/vabridgers/llvm-project-dev.git, branch: vla-fam-fixes. Everything seems ok on this branch (LITs pass, reproducers from https://bugs.llvm.org/show_bug.cgi?id=47272 and https://bugs.llvm.org/show_bug.cgi?id=28450 no longer crash). I can continue and push a change to Phabricator for review, or @Charusso and/or @balazske could finish this? I didn't want to just push an update without asking first :/ Cheers!

Jan 21 2021, 6:33 AM · Restricted Project

Jan 20 2021

martong accepted D94786: [clang][ASTImporter] Add support for importing CXXFoldExpr..

Thanks for the update!

Jan 20 2021, 9:20 AM · Restricted Project
martong added a comment to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.

It seems quite a challenge to hook the Preprocessor for all possible configurations for every CompilerInvocation.
The underlying machinery is somewhat complex and spaghetti to me.

Here is what I suggest:
For now, this expansion is better than the previous was. Macro expansions will work for the main TU even in any CTU configuration. For the imported TUs, it will just not expand any macros.
This is a regression in some way, but we should live with that until we implement it completely.
I think the users would prefer a non-crashing partial implementation to a crashing one.

If you think it's an appropriate, I will update the CTU tests to expect no macro expansion in any imported TU.
And also remove the XFAIL.
This way we could still target clang-12.

Do you think it's acceptable?
@xazax.hun @NoQ

Jan 20 2021, 8:42 AM · Restricted Project
martong accepted D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.

Nice work! Thanks!

Jan 20 2021, 8:36 AM · Restricted Project

Jan 19 2021

martong accepted D94786: [clang][ASTImporter] Add support for importing CXXFoldExpr..

Looks good to me! Thanks!

Jan 19 2021, 6:42 AM · Restricted Project
martong accepted D92797: APINotes: add initial stub of APINotesWriter.

Ping x 3

Jan 19 2021, 6:28 AM · Restricted Project
martong added a comment to D94067: [clang][ASTImporter] Fix a possible assertion failure `NeedsInjectedClassNameType(Decl)'..

@balazske
Thanks Balázs! Great work!

Jan 19 2021, 5:28 AM · Restricted Project

Dec 15 2020

martong accepted D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes..

Thanks for your work, I am glad that finally the Window bug is fixed with a simple solution.

Dec 15 2020, 2:28 PM · Restricted Project
martong added inline comments to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.
Dec 15 2020, 4:09 AM · Restricted Project
martong added inline comments to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.
Dec 15 2020, 4:05 AM · Restricted Project
martong added inline comments to D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.
Dec 15 2020, 4:04 AM · Restricted Project
martong added a comment to D93223: [analyzer] Create MacroExpansionContext member in AnalysisConsumer.

Looks quite straight-forward other than that new prototype.

Dec 15 2020, 3:57 AM · Restricted Project
martong added a comment to D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.

Nice and precise work! And special thanks for the unit tests. I've found some minor nits.

Dec 15 2020, 3:51 AM · Restricted Project
martong requested changes to D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes..
Dec 15 2020, 3:14 AM · Restricted Project
martong added a comment to D92797: APINotes: add initial stub of APINotesWriter.

Ping

Dec 15 2020, 2:45 AM · Restricted Project
martong added a comment to D92634: [Analyzer] Diagnose signed integer overflow.

I think it could be better to implement this check with a checker on PreStmt<BinaryOperator> and so on. And IMO, checkers have enough functionalities to report these problems.

Besides, the return value should be the exact value computed from the two integers, even unknown, rather than undefined. As the developers may overflow an integer on purpose.

Dec 15 2020, 2:31 AM · Restricted Project
martong added a comment to D92634: [Analyzer] Diagnose signed integer overflow.

No reviews => I will not contribute.

Dec 15 2020, 2:24 AM · Restricted Project

Dec 14 2020

martong added a comment to D92962: [ASTImporter] Fix import of a typedef that has an attribute.

Shafik, thanks for the review!

Dec 14 2020, 9:28 AM · Restricted Project
martong committed rG68f53960e17d: [ASTImporter] Fix import of a typedef that has an attribute (authored by martong).
[ASTImporter] Fix import of a typedef that has an attribute
Dec 14 2020, 9:27 AM
martong closed D92962: [ASTImporter] Fix import of a typedef that has an attribute.
Dec 14 2020, 9:27 AM · Restricted Project
martong accepted D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes..

Thanks for the update. I checked it, still looks good to me.

Dec 14 2020, 8:11 AM · Restricted Project

Dec 10 2020

martong added inline comments to D92962: [ASTImporter] Fix import of a typedef that has an attribute.
Dec 10 2020, 6:00 AM · Restricted Project
martong updated the diff for D92962: [ASTImporter] Fix import of a typedef that has an attribute.
  • Check the attribute in the test as well
Dec 10 2020, 5:59 AM · Restricted Project
martong added a comment to D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes..

I wouldn't worry much about the ASTMerge tests, those are mostly legacy and not actively used nowadays. (At one point we were thinking even to remove them because we could got rid of the ASTMergeAction which is exclusively used for testing.)
The main test infrastructure for the ASTImporter is in the unittests. Those tests are exercised even with -fdelayed-template-parsing and with -fms-compatibility. So I am fine with marking the ASTMerge tests with XFAIL on windows.

Dec 10 2020, 5:47 AM · Restricted Project
martong accepted D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes..

LGTM!

Dec 10 2020, 2:54 AM · Restricted Project

Dec 9 2020

martong requested review of D92962: [ASTImporter] Fix import of a typedef that has an attribute.
Dec 9 2020, 12:49 PM · Restricted Project
martong committed rGa5e6590b15b1: [ASTImporter] Support CXXDeductionGuideDecl with local typedef (authored by martong).
[ASTImporter] Support CXXDeductionGuideDecl with local typedef
Dec 9 2020, 12:25 PM
martong closed D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef.
Dec 9 2020, 12:25 PM · Restricted Project
martong updated the diff for D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef.
  • Remove not relevant param from test
Dec 9 2020, 11:41 AM · Restricted Project
martong added a comment to D92848: [lldb] Create a ASTImporterSharedState and pass it to all ASTImporterDelegates.

However, every single TU in LLDB starts being pretty much empty (we do have some dummy declarations in them, but none of them have any name conflicts with declarations in any other AST we import from). So technically the 'empty' LookupTable is accurate.

Dec 9 2020, 9:08 AM
martong added a comment to D92848: [lldb] Create a ASTImporterSharedState and pass it to all ASTImporterDelegates.

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

@martong I assume I can make a follow-up for removing all the ASTImporter code for situations without a shared state?

Yes, that'd would be super.
Other than that, if you could make sure that the ASTImporterLookupTable is initialized then noload_lookup would not be needed and we could simplify so much in clang::ASTImporter.

Dec 9 2020, 6:30 AM
martong added a comment to D92848: [lldb] Create a ASTImporterSharedState and pass it to all ASTImporterDelegates.

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

Dec 9 2020, 6:27 AM
martong added a comment to D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef.

Thanks for the review guys!

Dec 9 2020, 6:11 AM · Restricted Project
martong updated the diff for D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef.
  • Remove if (!DC || !LexicalDC)
Dec 9 2020, 6:08 AM · Restricted Project
martong added inline comments to D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis..
Dec 9 2020, 3:33 AM · Restricted Project

Dec 8 2020

martong committed rGfebe75032f6f: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints (authored by martong).
[analyzer][StdLibraryFunctionsChecker] Add more return value contraints
Dec 8 2020, 8:06 AM
martong closed D92771: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints.
Dec 8 2020, 8:06 AM · Restricted Project
martong added a comment to D92771: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints.

Thanks for the review!

Dec 8 2020, 8:03 AM · Restricted Project