[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

I see,t hank you. Please feel free to submit a patch - it seems like you 
already have a nice test case that shows the difference between two import 
options.


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.

It puts everything at the start of the marco expansion.

  `-FunctionDecl 0x12f5218 prev 0x12f4fa0  line:12:5 used 
foo 'int ()'
`-CompoundStmt 0x12f5600 
  |-DeclStmt 0x12f5508 
  | `-VarDecl 0x12f5470  col:15 used s 'struct S *' cinit
  |   `-ImplicitCastExpr 0x12f54f0  'struct S *' 
  | `-IntegerLiteral 0x12f54d0  'unsigned int' 2147516416
  `-ParenExpr 0x12f55e0  'int'
`-BinaryOperator 0x12f55b8  'int' '='
  |-MemberExpr 0x12f5560  'int' lvalue ->a 0x12f5378
  | `-ImplicitCastExpr 0x12f5548  'struct S *' 
  |   `-DeclRefExpr 0x12f5520  'struct S *' lvalue Var 0x12f5470 
's' 'struct S *'
  `-IntegerLiteral 0x12f5598  'int' 0


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hi Rafael,

Could you please show the AST we get with `getDecomposedExpansionLoc()`? This 
change can be an item for a separate patch.


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.
Herald added a subscriber: martong.

I encountered an issue caused by this change.

In the scenario as shown below the call to getFileLoc causes the startLoc of 
the BinaryOp to be after the endLoc, because the LHS (`s->a`) is located in the 
marco argument while the RHS (`0`) is located at the beginning of the marco. 
See below: `BinaryOperator 0x12f45b8 `

This is an issue because the diagnostics machinery asserts on correctly ordered 
sourceranges. Namely:

`llvm/tools/clang/lib/Frontend/TextDiagnostic.cpp:1015: void 
highlightRange(const clang::CharSourceRange&, unsigned int, clang::FileID, 
const {anonymous}::SourceColumnMap&, std::__cxx11::string&, const 
clang::SourceManager&, const clang::LangOptions&): Assertion 'StartColNo <= 
EndColNo && "Invalid range!"' failed.`

I have tested the solution proposed by @a.sidorin and it works for my tests and 
the regression tests still pass.

code:

  01|#include 
  02|
  03|
  04|#define CLR_BIT(reg) (reg = 0)
  05|
  06|
  07|struct S
  08|{
  09|int a;
  10|};
  11|
  12|int foo()
  13|{
  14|struct S *s = 0x80008000;
  15|CLR_BIT(s->a);
  16|}

original:

  `-FunctionDecl 0xd29850  line:12:5 foo 'int ()'
`-CompoundStmt 0xd312d8 
  |-DeclStmt 0xd311e0 
  | `-VarDecl 0xd299e0  col:15 used s 'struct S *' cinit
  |   `-ImplicitCastExpr 0xd29a60  'struct S *' 
  | `-IntegerLiteral 0xd29a40  'unsigned int' 2147516416
  `-ParenExpr 0xd312b8  'int'
`-BinaryOperator 0xd31290  'int' '='
  |-MemberExpr 0xd31238  'int' lvalue ->a 0xd297b8
  | `-ImplicitCastExpr 0xd31220  'struct S *' 
  |   `-DeclRefExpr 0xd311f8  'struct S *' lvalue Var 0xd299e0 
's' 'struct S *'
  `-IntegerLiteral 0xd31270  'int' 0

imported:

  `-FunctionDecl 0x12f4218 prev 0x12f3fa0  line:12:5 used 
foo 'int ()'
`-CompoundStmt 0x12f4600 
  |-DeclStmt 0x12f4508 
  | `-VarDecl 0x12f4470  col:15 used s 'struct S *' cinit
  |   `-ImplicitCastExpr 0x12f44f0  'struct S *' 
  | `-IntegerLiteral 0x12f44d0  'unsigned int' 2147516416
  `-ParenExpr 0x12f45e0  'int'
`-BinaryOperator 0x12f45b8  'int' '='
  |-MemberExpr 0x12f4560  'int' lvalue ->a 0x12f4378
  | `-ImplicitCastExpr 0x12f4548  'struct S *' 
  |   `-DeclRefExpr 0x12f4520  'struct S *' lvalue Var 
0x12f4470 's' 'struct S *'
  `-IntegerLiteral 0x12f4598  'int' 0


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Sean Callanan via cfe-commits]

spyffe added a comment.

Fixed the testcase to use _Nullable instead of __nullable, for Linux buildbots

  $ svn commit test
  Sendingtest/ASTMerge/Inputs/macro1.h
  Transmitting file data .done
  Committing transaction...
  Committed revision 286151.


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Sean Callanan via cfe-commits]

spyffe closed this revision.
spyffe added a comment.

  $ svn commit lib test
  Sendinglib/AST/ASTImporter.cpp
  Adding test/ASTMerge/Inputs/macro.modulemap
  Adding test/ASTMerge/Inputs/macro1.h
  Adding test/ASTMerge/Inputs/macro1.m
  Adding test/ASTMerge/Inputs/macro2.m
  Adding test/ASTMerge/macro.m
  Transmitting file data ..done
  Committing transaction...
  Committed revision 286144.


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Aleksei Sidorin via cfe-commits]

a.sidorin added a comment.

> Do you think it would be reasonable to take this diff the way it currently it 
> is, and start a new one that pulls all the input fiels into test-specific 
> subdirectories?
>  That way the desired layout of the `Inputs` directory will be clear to 
> future developers touching the source base.

I'm totally OK with a new diff. Nice suggestion. Thank you!


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Sean Callanan via cfe-commits]

spyffe added a comment.

That seems reasonable, and would go a long way toward cleaning up the `Inputs` 
and making clear exactly which inputs correspond to which test file.
Do you think it would be reasonable to take this diff the way it currently it 
is, and start a new one that pulls all the input fiels into test-specific 
subdirectories?
That way the desired layout of the `Inputs` directory will be clear to future 
developers touching the source base.


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Aleksei Sidorin via cfe-commits]

a.sidorin added a comment.

Hi Sean!
I meant that I'd like to have a layout like this:
macro-loc.m
Inputs/macro-loc/macro.modulemap
Inputs/macro-loc/macro1.h
Inputs/macro-loc/macro1.m
Inputs/macro-loc/macro2.m

By the way, I see that we use another workaround for this issue: we use 
`getDecomposedExpansionLoc()` instead of `getDecomposedLoc()` with removing 
previous line at all. I wonder if we are incorrect in our suggestion.


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Sean Callanan via cfe-commits]

spyffe added a comment.

Aleksei, thank you for your review.
I don't quite follow what you'd like me to do with the `Input` files, though.  
Some of them certainly appear to be input files in the same way that all the 
other files in `Inputs` are.  Are you suggesting that I move `macro.modulemap` 
and `macro1.h` somewhere else (say, `Modules/`) or are you making a general 
comment about the layout of all of the tests?


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Aleksei Sidorin via cfe-commits]

a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.

Looks good. But I think it will be good to put the files in Input to a separate 
directory.


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Argyrios Kyrtzidis via cfe-commits]

akyrtzi added a comment.

LGTM.


https://reviews.llvm.org/D26054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Sean Callanan via cfe-commits]

spyffe updated this revision to Diff 76213.
spyffe added a comment.

Updated the corresponding comment.


https://reviews.llvm.org/D26054

Files:
  lib/AST/ASTImporter.cpp
  test/ASTMerge/Inputs/macro.modulemap
  test/ASTMerge/Inputs/macro1.h
  test/ASTMerge/Inputs/macro1.m
  test/ASTMerge/Inputs/macro2.m
  test/ASTMerge/macro.m


Index: test/ASTMerge/macro.m
===
--- test/ASTMerge/macro.m
+++ test/ASTMerge/macro.m
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/cache
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache 
-fmodule-map-file=%S/Inputs/macro.modulemap -I%S/Inputs -emit-pch -o %t.1.ast 
%S/Inputs/macro1.m
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache 
-fmodule-map-file=%S/Inputs/macro.modulemap -I%S/Inputs -emit-pch -o %t.2.ast 
%S/Inputs/macro2.m
+// RUN: %clang_cc1 -fmodules -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only -verify %s
+// expected-no-diagnostics
Index: test/ASTMerge/Inputs/macro2.m
===
--- test/ASTMerge/Inputs/macro2.m
+++ test/ASTMerge/Inputs/macro2.m
@@ -0,0 +1,5 @@
+void foo();
+
+void bar() {
+  foo();
+}
Index: test/ASTMerge/Inputs/macro1.m
===
--- test/ASTMerge/Inputs/macro1.m
+++ test/ASTMerge/Inputs/macro1.m
@@ -0,0 +1,5 @@
+@import macro1;
+
+void foo() {
+  maybeNull(0, 0);
+}
Index: test/ASTMerge/Inputs/macro1.h
===
--- test/ASTMerge/Inputs/macro1.h
+++ test/ASTMerge/Inputs/macro1.h
@@ -0,0 +1,5 @@
+typedef void *VoidRef;
+
+void maybeNull(
+  int i,
+  __nullable VoidRef *__nullable);
Index: test/ASTMerge/Inputs/macro.modulemap
===
--- test/ASTMerge/Inputs/macro.modulemap
+++ test/ASTMerge/Inputs/macro.modulemap
@@ -0,0 +1,4 @@
+module macro1 [extern_c] {
+  header "macro1.h"
+  export *
+}
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -6943,10 +6943,10 @@
 
   SourceManager  = FromContext.getSourceManager();
   
-  // For now, map everything down to its spelling location, so that we
+  // For now, map everything down to its file location, so that we
   // don't have to import macro expansions.
   // FIXME: Import macro expansions!
-  FromLoc = FromSM.getSpellingLoc(FromLoc);
+  FromLoc = FromSM.getFileLoc(FromLoc);
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
   SourceManager  = ToContext.getSourceManager();
   FileID ToFileID = Import(Decomposed.first);


Index: test/ASTMerge/macro.m
===
--- test/ASTMerge/macro.m
+++ test/ASTMerge/macro.m
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/cache
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fmodule-map-file=%S/Inputs/macro.modulemap -I%S/Inputs -emit-pch -o %t.1.ast %S/Inputs/macro1.m
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fmodule-map-file=%S/Inputs/macro.modulemap -I%S/Inputs -emit-pch -o %t.2.ast %S/Inputs/macro2.m
+// RUN: %clang_cc1 -fmodules -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
Index: test/ASTMerge/Inputs/macro2.m
===
--- test/ASTMerge/Inputs/macro2.m
+++ test/ASTMerge/Inputs/macro2.m
@@ -0,0 +1,5 @@
+void foo();
+
+void bar() {
+  foo();
+}
Index: test/ASTMerge/Inputs/macro1.m
===
--- test/ASTMerge/Inputs/macro1.m
+++ test/ASTMerge/Inputs/macro1.m
@@ -0,0 +1,5 @@
+@import macro1;
+
+void foo() {
+  maybeNull(0, 0);
+}
Index: test/ASTMerge/Inputs/macro1.h
===
--- test/ASTMerge/Inputs/macro1.h
+++ test/ASTMerge/Inputs/macro1.h
@@ -0,0 +1,5 @@
+typedef void *VoidRef;
+
+void maybeNull(
+  int i,
+  __nullable VoidRef *__nullable);
Index: test/ASTMerge/Inputs/macro.modulemap
===
--- test/ASTMerge/Inputs/macro.modulemap
+++ test/ASTMerge/Inputs/macro.modulemap
@@ -0,0 +1,4 @@
+module macro1 [extern_c] {
+  header "macro1.h"
+  export *
+}
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -6943,10 +6943,10 @@
 
   SourceManager  = FromContext.getSourceManager();
   
-  // For now, map everything down to its spelling location, so that we
+  // For now, map everything down to its file location, so that we
   // don't have to import macro expansions.
   // FIXME: Import macro expansions!
-  FromLoc = FromSM.getSpellingLoc(FromLoc);
+  FromLoc = FromSM.getFileLoc(FromLoc);
   std::pair 

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

[Sean Callanan via cfe-commits]

spyffe created this revision.
spyffe added reviewers: bruno, akyrtzi, a.sidorin.
spyffe added a subscriber: cfe-commits.

When the `ASTImporter`imports a source location, it avoids importing macro 
expansions by calling `getSpellingLoc()`.  That's great in most cases, but for 
macros defined in the '' source file, the source file is invalid and 
does not import correctly, causing an assertion failure (the assertion is 
`Invalid SLocOffset or bad function choice`).

A more reliable way to avoid this is to use `getFileLoc()`, which does not 
return built-in locations.  This avoids the crash but still preserves valid 
source locations.

I've added a testcase that covers the previously crashing scenario.


https://reviews.llvm.org/D26054

Files:
  lib/AST/ASTImporter.cpp
  test/ASTMerge/Inputs/macro.modulemap
  test/ASTMerge/Inputs/macro1.h
  test/ASTMerge/Inputs/macro1.m
  test/ASTMerge/Inputs/macro2.m
  test/ASTMerge/macro.m


Index: test/ASTMerge/macro.m
===
--- test/ASTMerge/macro.m
+++ test/ASTMerge/macro.m
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/cache
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache 
-fmodule-map-file=%S/Inputs/macro.modulemap -I%S/Inputs -emit-pch -o %t.1.ast 
%S/Inputs/macro1.m
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache 
-fmodule-map-file=%S/Inputs/macro.modulemap -I%S/Inputs -emit-pch -o %t.2.ast 
%S/Inputs/macro2.m
+// RUN: %clang_cc1 -fmodules -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only -verify %s
+// expected-no-diagnostics
Index: test/ASTMerge/Inputs/macro2.m
===
--- test/ASTMerge/Inputs/macro2.m
+++ test/ASTMerge/Inputs/macro2.m
@@ -0,0 +1,5 @@
+void foo();
+
+void bar() {
+  foo();
+}
Index: test/ASTMerge/Inputs/macro1.m
===
--- test/ASTMerge/Inputs/macro1.m
+++ test/ASTMerge/Inputs/macro1.m
@@ -0,0 +1,5 @@
+@import macro1;
+
+void foo() {
+  maybeNull(0, 0);
+}
Index: test/ASTMerge/Inputs/macro1.h
===
--- test/ASTMerge/Inputs/macro1.h
+++ test/ASTMerge/Inputs/macro1.h
@@ -0,0 +1,5 @@
+typedef void *VoidRef;
+
+void maybeNull(
+  int i,
+  __nullable VoidRef *__nullable);
Index: test/ASTMerge/Inputs/macro.modulemap
===
--- test/ASTMerge/Inputs/macro.modulemap
+++ test/ASTMerge/Inputs/macro.modulemap
@@ -0,0 +1,4 @@
+module macro1 [extern_c] {
+  header "macro1.h"
+  export *
+}
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -6946,7 +6946,7 @@
   // For now, map everything down to its spelling location, so that we
   // don't have to import macro expansions.
   // FIXME: Import macro expansions!
-  FromLoc = FromSM.getSpellingLoc(FromLoc);
+  FromLoc = FromSM.getFileLoc(FromLoc);
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
   SourceManager  = ToContext.getSourceManager();
   FileID ToFileID = Import(Decomposed.first);


Index: test/ASTMerge/macro.m
===
--- test/ASTMerge/macro.m
+++ test/ASTMerge/macro.m
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/cache
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fmodule-map-file=%S/Inputs/macro.modulemap -I%S/Inputs -emit-pch -o %t.1.ast %S/Inputs/macro1.m
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fmodule-map-file=%S/Inputs/macro.modulemap -I%S/Inputs -emit-pch -o %t.2.ast %S/Inputs/macro2.m
+// RUN: %clang_cc1 -fmodules -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
Index: test/ASTMerge/Inputs/macro2.m
===
--- test/ASTMerge/Inputs/macro2.m
+++ test/ASTMerge/Inputs/macro2.m
@@ -0,0 +1,5 @@
+void foo();
+
+void bar() {
+  foo();
+}
Index: test/ASTMerge/Inputs/macro1.m
===
--- test/ASTMerge/Inputs/macro1.m
+++ test/ASTMerge/Inputs/macro1.m
@@ -0,0 +1,5 @@
+@import macro1;
+
+void foo() {
+  maybeNull(0, 0);
+}
Index: test/ASTMerge/Inputs/macro1.h
===
--- test/ASTMerge/Inputs/macro1.h
+++ test/ASTMerge/Inputs/macro1.h
@@ -0,0 +1,5 @@
+typedef void *VoidRef;
+
+void maybeNull(
+  int i,
+  __nullable VoidRef *__nullable);
Index: test/ASTMerge/Inputs/macro.modulemap
===
--- test/ASTMerge/Inputs/macro.modulemap
+++ test/ASTMerge/Inputs/macro.modulemap
@@ -0,0 +1,4 @@
+module macro1 [extern_c] {
+  header "macro1.h"
+  export *
+}
Index: lib/AST/ASTImporter.cpp
===
---