Page MenuHomePhabricator

martong (Gabor Marton)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 10 2017, 8:01 AM (62 w, 15 h)

Recent Activity

Today

martong added a reviewer for D55646: [ASTImporter] Make ODR diagnostics warning by default: a_sidorin.
Tue, Dec 18, 2:48 AM

Yesterday

martong added a comment to D53708: [ASTImporter] Add importer specific lookup.

This is a perhaps a naive comment, but why is localUncachedLookup used
instead of noload_lookup ? There is a fixme dating from 2013 stating
that noload_lookup should be used instead.

This is not a naive question. I was wondering myself on the very same thing when I started working on the ASTImporter at 2017.
I think when noload_lookup was introduced the author of that function tried to replace localUncachedLookup in ASTImporter, but that broke some tests. The major difference between the two functions is that the latter can find declarations even if they are not stored in the LookupPtr. This is rather unusual from the C/C++ point of view of lookup, and not working in all cases (see the introduction of this patch). Ironically, I can't just get rid of localUncachedLookup because it would break some LLDB test cases.

Ah I think I understand. For my understanding (and please correct me if I am wrong here):

localUncachedLookup will first try to do a normal lookup, and if that fails it will try to look in the declarations in the declaration context.
Now some declarations (including declarations which are not NamedDecl and declarations which matches shouldBeHidden)
will not be added to the LookupPtr (via makeDeclVisibleInContextWithFlags) and so will not be found though the normal lookup mechanism. You therefore need to use localUncachedLookup to find these.

Does this make sense ?

Mon, Dec 17, 7:00 AM
martong added a comment to D44100: [ASTImporter] Reorder fields after structure import is finished.

Alexey, simply put, the crux of the problem is this:
The imported decl context is different than what we get by calling the getDeclContext() on an imported field/friend of the "from" decl context.

Mon, Dec 17, 6:46 AM
martong added a comment to D53708: [ASTImporter] Add importer specific lookup.

This is a perhaps a naive comment, but why is localUncachedLookup used

instead of noload_lookup ? There is a fixme dating from 2013 stating
that noload_lookup should be used instead.

Mon, Dec 17, 6:06 AM
martong committed rC349351: [ASTImporter] Add importer specific lookup.
[ASTImporter] Add importer specific lookup
Mon, Dec 17, 5:56 AM
martong committed rL349351: [ASTImporter] Add importer specific lookup.
[ASTImporter] Add importer specific lookup
Mon, Dec 17, 5:56 AM
martong closed D53708: [ASTImporter] Add importer specific lookup.
Mon, Dec 17, 5:56 AM
martong updated the diff for D53708: [ASTImporter] Add importer specific lookup.
  • Rebase to master (trunk)
Mon, Dec 17, 5:06 AM
martong committed rL349349: [ASTImporter] Fix redecl chain of classes and class templates.
[ASTImporter] Fix redecl chain of classes and class templates
Mon, Dec 17, 4:45 AM
martong committed rC349349: [ASTImporter] Fix redecl chain of classes and class templates.
[ASTImporter] Fix redecl chain of classes and class templates
Mon, Dec 17, 4:45 AM
martong closed D53655: [ASTImporter] Fix redecl chain of classes and class templates.
Mon, Dec 17, 4:45 AM
martong added a comment to D53699: [ASTImporter] Fix inequality of functions with different attributes.

Ping @shafik, I have addressed you comments, could you please take another look? If you don't have any objections, could you please approve?

Mon, Dec 17, 2:13 AM
martong added a comment to D55280: [CTU] Make loadExternalAST return with non nullptr on success.

Ping @a_sidorin Could you please take another look and consider my previous comment?

Mon, Dec 17, 2:10 AM

Fri, Dec 14

martong added a comment to D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer.

Hi Rafael,

Fri, Dec 14, 7:54 AM

Thu, Dec 13

martong updated the diff for D53699: [ASTImporter] Fix inequality of functions with different attributes.
  • Add more tests and simplify comment
Thu, Dec 13, 7:11 AM
martong added inline comments to D53699: [ASTImporter] Fix inequality of functions with different attributes.
Thu, Dec 13, 7:11 AM
martong added a comment to D55646: [ASTImporter] Make ODR diagnostics warning by default.

The primary reason why want these to be Warnings instead of Errors because there are many false positive ODR Errors at the moment. These are not fatal errors but there is a maximum error count which once reached the whole process (analysis) is aborted.
Usually, such false positives are related to a wrong import of redeclaration chains or existing errors in the structural match logic.

Thu, Dec 13, 5:37 AM

Wed, Dec 12

martong committed rC348923: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull.
[ASTImporter] Remove import of definition from GetAlreadyImportedOrNull
Wed, Dec 12, 3:26 AM
martong committed rL348923: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull.
[ASTImporter] Remove import of definition from GetAlreadyImportedOrNull
Wed, Dec 12, 3:26 AM
martong closed D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull.
Wed, Dec 12, 3:26 AM
martong added a comment to D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull.

We need to make sure we are running the lldb test suite before committing and watching the bots to make sure the commit does not break them.

Wed, Dec 12, 3:22 AM

Tue, Dec 11

martong added a reviewer for D53699: [ASTImporter] Fix inequality of functions with different attributes: shafik.
Tue, Dec 11, 8:08 AM

Mon, Dec 10

martong added a comment to D44100: [ASTImporter] Reorder fields after structure import is finished.

Alexey, good news! I have finished the testing of the above patch applied on top of 174545. Neither is a regression on Linux nor on macOS. (ASTTests, check-clang-astmerge, check-clang-import, check-clang-analysis, check-lldb).

Mon, Dec 10, 2:06 PM
martong added a comment to D44100: [ASTImporter] Reorder fields after structure import is finished.
Mon, Dec 10, 7:36 AM
martong added a comment to D44100: [ASTImporter] Reorder fields after structure import is finished.

The below diff on top of your patch successfully handles the failure with the TestCModules.py LLDB testcase:

diff --git a/lib/AST/ASTImporter.cpp b/lib/AST/ASTImporter.cpp
index 05fec7f943..e6fb590025 100644
--- a/lib/AST/ASTImporter.cpp
+++ b/lib/AST/ASTImporter.cpp
@@ -1695,15 +1695,22 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   // LoadFieldsFromExternalStorage().
   auto ImportedDC = import(cast<Decl>(FromDC));
   assert(ImportedDC);
-  auto *ToRD = cast<RecordDecl>(*ImportedDC);
+  RecordDecl *ToRD = nullptr;
   for (auto *D : FromRD->decls()) {
     if (isa<FieldDecl>(D) || isa<FriendDecl>(D)) {
       Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
-      assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
-      ToRD->removeDecl(ToD);
+      if (!ToRD)
+        ToRD = cast<RecordDecl>(ToD->getDeclContext());
+      else
+        assert(ToRD == ToD->getDeclContext());
+      if(ToRD->containsDecl(ToD))
+        ToRD->removeDecl(ToD);
     }
   }
Mon, Dec 10, 7:31 AM
martong added a comment to D53655: [ASTImporter] Fix redecl chain of classes and class templates.

@rsmith Please raise any objections until Dec 14 (or if this deadline is too strict). I'd like to commit this next week latest so it can get in still this year.

Mon, Dec 10, 1:48 AM

Fri, Dec 7

martong committed rC348614: [CTU] test/Analysis/ctu-main.cpp Attempt to fix failing windows bot.
[CTU] test/Analysis/ctu-main.cpp Attempt to fix failing windows bot
Fri, Dec 7, 9:39 AM
martong committed rL348614: [CTU] test/Analysis/ctu-main.cpp Attempt to fix failing windows bot.
[CTU] test/Analysis/ctu-main.cpp Attempt to fix failing windows bot
Fri, Dec 7, 9:39 AM
martong committed rC348610: [CTU] Add triple/lang mismatch handling.
[CTU] Add triple/lang mismatch handling
Fri, Dec 7, 8:36 AM
martong committed rL348610: [CTU] Add triple/lang mismatch handling.
[CTU] Add triple/lang mismatch handling
Fri, Dec 7, 8:36 AM
martong closed D55134: [CTU] Add triple/lang mismatch handling.
Fri, Dec 7, 8:36 AM
martong committed rC348609: [CTU] test/Analysis/ctu-main.cpp Attempt to fix failing windows bot.
[CTU] test/Analysis/ctu-main.cpp Attempt to fix failing windows bot
Fri, Dec 7, 8:30 AM
martong committed rL348609: [CTU] test/Analysis/ctu-main.cpp Attempt to fix failing windows bot.
[CTU] test/Analysis/ctu-main.cpp Attempt to fix failing windows bot
Fri, Dec 7, 8:30 AM
martong committed rC348605: [CTU] Add more lit tests and better error handling.
[CTU] Add more lit tests and better error handling
Fri, Dec 7, 8:09 AM
martong committed rL348605: [CTU] Add more lit tests and better error handling.
[CTU] Add more lit tests and better error handling
Fri, Dec 7, 8:09 AM
martong closed D55131: [CTU] Add more lit tests and better error handling.
Fri, Dec 7, 8:09 AM
martong committed rC348594: [CTU] Add DisplayCTUProgress analyzer switch.
[CTU] Add DisplayCTUProgress analyzer switch
Fri, Dec 7, 6:59 AM
martong committed rL348594: [CTU] Add DisplayCTUProgress analyzer switch.
[CTU] Add DisplayCTUProgress analyzer switch
Fri, Dec 7, 6:59 AM
martong closed D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.
Fri, Dec 7, 6:59 AM
martong updated the diff for D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.
  • Remove 'ANALYZE ' prefix from the log message
Fri, Dec 7, 6:33 AM
martong added a comment to D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.

While Static Analyzer is the only client of CTU library at the moment, we might have more in the future. I would not use the phrase ANALYZE in the log message. Once this is resolved the rest looks good.

Fri, Dec 7, 6:32 AM
martong committed rC348587: [CTU] Eliminate race condition in CTU lit tests.
[CTU] Eliminate race condition in CTU lit tests
Fri, Dec 7, 4:32 AM
martong committed rL348587: [CTU] Eliminate race condition in CTU lit tests.
[CTU] Eliminate race condition in CTU lit tests
Fri, Dec 7, 4:32 AM
martong closed D55129: [CTU] Eliminate race condition in CTU lit tests.
Fri, Dec 7, 4:32 AM
martong committed rC348586: [CTU] Add asserts to protect invariants.
[CTU] Add asserts to protect invariants
Fri, Dec 7, 4:24 AM
martong committed rL348586: [CTU] Add asserts to protect invariants.
[CTU] Add asserts to protect invariants
Fri, Dec 7, 4:24 AM
martong closed D55132: [CTU] Add asserts to protect invariants.
Fri, Dec 7, 4:24 AM
martong added inline comments to D55133: [CTU] Add statistics.
Fri, Dec 7, 4:03 AM
martong committed rC348584: [CTU] Add statistics.
[CTU] Add statistics
Fri, Dec 7, 3:58 AM
martong committed rL348584: [CTU] Add statistics.
[CTU] Add statistics
Fri, Dec 7, 3:58 AM
martong closed D55133: [CTU] Add statistics.
Fri, Dec 7, 3:58 AM
martong added inline comments to D55131: [CTU] Add more lit tests and better error handling.
Fri, Dec 7, 3:45 AM
martong updated the diff for D55131: [CTU] Add more lit tests and better error handling.
  • Rename externalFnMap files
Fri, Dec 7, 3:45 AM

Thu, Dec 6

martong added a comment to D44100: [ASTImporter] Reorder fields after structure import is finished.

Hey Alexey,

Thu, Dec 6, 9:53 AM
martong added a comment to D55280: [CTU] Make loadExternalAST return with non nullptr on success.

Hi Aleksey,

Thu, Dec 6, 7:56 AM

Wed, Dec 5

martong added inline comments to D55131: [CTU] Add more lit tests and better error handling.
Wed, Dec 5, 8:45 AM
martong added inline comments to D55280: [CTU] Make loadExternalAST return with non nullptr on success.
Wed, Dec 5, 8:42 AM
martong retitled D55280: [CTU] Make loadExternalAST return with non nullptr on success from [CTU] Remove redundant error check to [CTU] Make loadExternalAST return with non nullptr on success.
Wed, Dec 5, 2:53 AM
martong added a comment to D55280: [CTU] Make loadExternalAST return with non nullptr on success.

@Szelethus @balazske Thanks for your review! The meantime I have discussed with @xazax.hun that actually the called ASTUnit::LoadFromASTFile function inside loadExternalAST may return with a nullptr. So, the best is to handle that inside loadExternalAST.

Wed, Dec 5, 2:52 AM
martong updated the diff for D55280: [CTU] Make loadExternalAST return with non nullptr on success.
  • Return an error from loadExternalAST in case of nullptr
Wed, Dec 5, 2:45 AM

Tue, Dec 4

martong added a comment to D55134: [CTU] Add triple/lang mismatch handling.

We should probably prefer %clang_analyze_cc1 to %clang_cc1 -analyze here too.

Tue, Dec 4, 12:14 PM
martong updated the diff for D55134: [CTU] Add triple/lang mismatch handling.
  • Use clang_analyze_cc1
Tue, Dec 4, 12:13 PM
martong updated the diff for D55134: [CTU] Add triple/lang mismatch handling.
  • Add a case to emitCrossTUDiagnostics
Tue, Dec 4, 12:11 PM
martong added inline comments to D55134: [CTU] Add triple/lang mismatch handling.
Tue, Dec 4, 12:11 PM
martong updated the diff for D55134: [CTU] Add triple/lang mismatch handling.
  • Forgot to rename err_ctu_incompat_triple -> warn_ctu_incompat_triple
Tue, Dec 4, 10:33 AM
martong added inline comments to D55134: [CTU] Add triple/lang mismatch handling.
Tue, Dec 4, 10:31 AM
martong updated the diff for D55134: [CTU] Add triple/lang mismatch handling.
  • Diagnose a warning (which may be upgradable to an error)
Tue, Dec 4, 10:31 AM
martong added a comment to D53655: [ASTImporter] Fix redecl chain of classes and class templates.

@rsmith ping

Tue, Dec 4, 9:57 AM
martong updated the diff for D55129: [CTU] Eliminate race condition in CTU lit tests.
  • Use rm, mkdir and break long RUN lines
Tue, Dec 4, 9:54 AM
martong created D55280: [CTU] Make loadExternalAST return with non nullptr on success.
Tue, Dec 4, 9:40 AM
martong added a comment to D55133: [CTU] Add statistics.

> Sorry, but I don't understand the meaning of some options. Could you please explain what are NumNoUnit and NumNotInOtherTU and what is the difference between them?

Tue, Dec 4, 9:26 AM
martong updated the diff for D55133: [CTU] Add statistics.
  • Remove braces
Tue, Dec 4, 9:24 AM
martong added a comment to D55133: [CTU] Add statistics.

Sorry, but I don't understand the meaning of some options. Could you please explain what are NumNoUnit and NumNotInOtherTU and what is the difference between them?

Tue, Dec 4, 9:16 AM
martong updated the diff for D55133: [CTU] Add statistics.
  • Remove NumNoUnit
Tue, Dec 4, 9:15 AM
martong added a comment to D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.

should be an -analyzer-config option.

Tue, Dec 4, 8:52 AM
martong updated the diff for D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.
  • Use clang_analyze_cc1
  • Change to be an analyzer config option
Tue, Dec 4, 8:52 AM
martong added a comment to D55131: [CTU] Add more lit tests and better error handling.

Also, maybe it'd be worth making a CTU directory under test/Analysis for CTU related test files.

Tue, Dec 4, 6:33 AM
martong added inline comments to D55131: [CTU] Add more lit tests and better error handling.
Tue, Dec 4, 6:30 AM
martong updated the diff for D55131: [CTU] Add more lit tests and better error handling.
  • Break long RUN lines
  • Clang format the test files
  • Use consistent naming style
  • Remove braces
Tue, Dec 4, 6:30 AM
martong added inline comments to D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.
Tue, Dec 4, 3:14 AM
martong updated the diff for D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.
  • Break long RUN line
Tue, Dec 4, 3:14 AM
martong updated the diff for D55134: [CTU] Add triple/lang mismatch handling.
  • Break long RUN lines
Tue, Dec 4, 3:06 AM
martong updated the diff for D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.
  • Change the CTU progress message in the test too
Tue, Dec 4, 2:49 AM
martong updated the diff for D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.
  • Rename AnalyzerDisplayCtuProgress to Opts.AnalyzerDisplayCTUProgress
  • Change the CTU progress message
Tue, Dec 4, 2:46 AM
martong added a comment to D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.

Also, almost everywhere CTU is capitalized, so I guess it should be in the field name too.

Tue, Dec 4, 2:46 AM

Mon, Dec 3

martong added a comment to D55134: [CTU] Add triple/lang mismatch handling.

Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be an issue there?

Mon, Dec 3, 9:26 AM
martong added a comment to D55134: [CTU] Add triple/lang mismatch handling.

Thanks for the review! I have updated the patch according to your comments.

Mon, Dec 3, 9:22 AM
martong updated the diff for D55134: [CTU] Add triple/lang mismatch handling.
  • Address review comments
Mon, Dec 3, 9:22 AM

Sat, Dec 1

martong added a comment to D44100: [ASTImporter] Reorder fields after structure import is finished.

I had a chance to have a quick look into this on a borrowed Mac Book. Seems like the assertion which was related to the revert is not existent anymore, but a new assertion came in. Next week I'll have time to debug that and I'll come back to you with what I find.

Sat, Dec 1, 7:32 AM

Fri, Nov 30

martong updated the diff for D55134: [CTU] Add triple/lang mismatch handling.
  • Add lit tests
Fri, Nov 30, 11:31 AM
martong created D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch.
Fri, Nov 30, 10:12 AM
martong created D55134: [CTU] Add triple/lang mismatch handling.
Fri, Nov 30, 9:55 AM
martong created D55133: [CTU] Add statistics.
Fri, Nov 30, 9:37 AM
martong created D55132: [CTU] Add asserts to protect invariants.
Fri, Nov 30, 9:32 AM
martong created D55131: [CTU] Add more lit tests and better error handling.
Fri, Nov 30, 9:25 AM
martong created D55129: [CTU] Eliminate race condition in CTU lit tests.
Fri, Nov 30, 8:19 AM
martong added a comment to D53708: [ASTImporter] Add importer specific lookup.

Hey Alexei,

Fri, Nov 30, 5:47 AM
martong updated the diff for D53708: [ASTImporter] Add importer specific lookup.
  • Address Alexei's comments
  • Rename FindDeclsInToCtx to findDeclsInToCtx
  • Remove superfluous spaces from stringliterals
  • Remove unused header
  • Remove empty test
  • Return nullptr and FindInDeclListOfDC -> findInDeclListOfDC
  • Use SmallSetVector instead of SmallPtrSet
Fri, Nov 30, 5:42 AM
martong added inline comments to D53708: [ASTImporter] Add importer specific lookup.
Fri, Nov 30, 5:41 AM

Wed, Nov 28

martong added a comment to D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter..

I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too). To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on. I'm worried about the broader trend of landing patches to ASTImporter without proper reviews and testing as I am of this very instance. I really think you should consider asking reviews to somebody who's familiar with clang and test lldb as it's the main client of this functionality we have in tree.

Wed, Nov 28, 2:48 PM