[clang][SourceManager] Use getFileLoc when computing getPresumedLoc
#168402
+22
−23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change moves away from using expansion location when computing presumed location.
SergejSalnikov
requested review from
QuietMisdreavus and
daniel-grumberg
as code owners
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Sergej Salnikov (SergejSalnikov) ChangesNow the files location is used for macro expansions. This provides more accurate location when reporting compilation errors. Move from (it's a rollforward of #166255 with fixed test) Full diff: https://github.com/llvm/llvm-project/pull/168402.diff 8 Files Affected:
diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
index bc9e97863556d..f15257a760b8c 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -1464,8 +1464,9 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// directives. This provides a view on the data that a user should see
/// in diagnostics, for example.
///
- /// Note that a presumed location is always given as the expansion point of
- /// an expansion location, not at the spelling location.
+ /// If \p Loc is a macro expansion location, the presumed location
+ /// computation uses the spelling location for macro arguments and the
+ /// expansion location for other macro expansions.
///
/// \returns The presumed location of the specified SourceLocation. If the
/// presumed location cannot be calculated (e.g., because \p Loc is invalid
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index b6cc6ec9365f5..767a765ae4261 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -1435,7 +1435,7 @@ PresumedLoc SourceManager::getPresumedLoc(SourceLocation Loc,
if (Loc.isInvalid()) return PresumedLoc();
// Presumed locations are always for expansion points.
- FileIDAndOffset LocInfo = getDecomposedExpansionLoc(Loc);
+ FileIDAndOffset LocInfo = getDecomposedLoc(getFileLoc(Loc));
bool Invalid = false;
const SLocEntry &Entry = getSLocEntry(LocInfo.first, &Invalid);
diff --git a/clang/test/Analysis/plist-macros-with-expansion.cpp b/clang/test/Analysis/plist-macros-with-expansion.cpp
index d57bb0f2dd265..d9a2f94055593 100644
--- a/clang/test/Analysis/plist-macros-with-expansion.cpp
+++ b/clang/test/Analysis/plist-macros-with-expansion.cpp
@@ -405,14 +405,14 @@ void commaInBracketsTest() {
code
void commaInBracesTest() {
- PASTE_CODE({ // expected-warning{{Dereference of null pointer}}
+ PASTE_CODE({
// NOTE: If we were to add a new variable here after a comma, we'd get a
// compilation error, so this test is mainly here to show that this was also
// investigated.
//
// int *ptr = nullptr, a;
int *ptr = nullptr;
- *ptr = 5;
+ *ptr = 5; // expected-warning{{Dereference of null pointer}}
})
}
@@ -425,14 +425,14 @@ void commaInBracesTest() {
// CHECK-NEXT: <key>col</key><integer>3</integer>
// CHECK-NEXT: <key>file</key><integer>0</integer>
// CHECK-NEXT: </dict>
-// CHECK-NEXT: <key>name</key><string>PASTE_CODE({ // expected-
+// CHECK-NEXT: <key>name</key><string>PASTE_CODE({
// CHECK-NEXT: // NOTE: If we were to add a new variable here after a comma, we'd get a
// CHECK-NEXT: // compilation error, so this test is mainly here to show that this was also
// CHECK-NEXT: // investigated.
// CHECK-NEXT: //
// CHECK-NEXT: // int *ptr = nullptr, a;
// CHECK-NEXT: int *ptr = nullptr;
-// CHECK-NEXT: *ptr = 5;
+// CHECK-NEXT: *ptr = 5; // expected-
// CHECK-NEXT: })</string>
// CHECK-NEXT: <key>expansion</key><string>{int *ptr =nullptr ;*ptr =5;}</string>
// CHECK-NEXT: </dict>
diff --git a/clang/test/C/C23/n2350.c b/clang/test/C/C23/n2350.c
index af0ca6d79be5e..96b8c511d5716 100644
--- a/clang/test/C/C23/n2350.c
+++ b/clang/test/C/C23/n2350.c
@@ -47,11 +47,10 @@ int struct_in_second_param(void) {
int macro(void) {
return offsetof(struct A // cpp-error {{'A' cannot be defined in a type specifier}} \
- expected-warning 2 {{defining a type within 'offsetof' is a C23 extension}}
+ expected-warning {{defining a type within 'offsetof' is a C23 extension}}
{
int a;
- struct B // verifier seems to think the error is emitted by the macro
- // In fact the location of the error is "B" on the line above
+ struct B // expected-warning {{defining a type within 'offsetof' is a C23 extension}}
{
int c;
int d;
diff --git a/clang/test/ExtractAPI/macro_undefined.c b/clang/test/ExtractAPI/macro_undefined.c
index 7bb50af380c24..1d697db1e1613 100644
--- a/clang/test/ExtractAPI/macro_undefined.c
+++ b/clang/test/ExtractAPI/macro_undefined.c
@@ -89,7 +89,7 @@ FUNC_GEN(bar, const int *, unsigned);
},
"location": {
"position": {
- "character": 0,
+ "character": 9,
"line": 2
},
"uri": "file://INPUT_DIR/input.h"
@@ -241,7 +241,7 @@ FUNC_GEN(bar, const int *, unsigned);
},
"location": {
"position": {
- "character": 0,
+ "character": 9,
"line": 3
},
"uri": "file://INPUT_DIR/input.h"
diff --git a/clang/test/FixIt/format.cpp b/clang/test/FixIt/format.cpp
index d663c0fb35e13..db642b60ffd95 100644
--- a/clang/test/FixIt/format.cpp
+++ b/clang/test/FixIt/format.cpp
@@ -56,9 +56,9 @@ void a(N::E NEVal, S *SPtr, S &SRef) {
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:17}:")"
- LOG( // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
+ LOG(
"%d",
- SPtr->Type
+ SPtr->Type // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
);
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:17}:")"
@@ -68,8 +68,8 @@ void a(N::E NEVal, S *SPtr, S &SRef) {
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:")"
- LOG("%d", // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
- SRef.Type);
+ LOG("%d",
+ SRef.Type); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:")"
diff --git a/clang/test/Preprocessor/macro_arg_directive.c b/clang/test/Preprocessor/macro_arg_directive.c
index 929a03d70d025..c612aa545a2a9 100644
--- a/clang/test/Preprocessor/macro_arg_directive.c
+++ b/clang/test/Preprocessor/macro_arg_directive.c
@@ -18,7 +18,7 @@ void fail(const char *);
({ int result = 0; __VA_ARGS__; if (!result) { fail(#__VA_ARGS__); }; result })
static inline int f(int k) {
- return MUNCH( // expected-error {{expected ')'}} expected-note {{to match this '('}} expected-error {{returning 'void'}} expected-note {{expansion of macro 'MUNCH' requested here}}
+ return MUNCH( // expected-note {{to match this '('}} expected-error {{returning 'void'}} expected-note {{expansion of macro 'MUNCH' requested here}}
if (k < 3)
result = 24;
else if (k > 4)
@@ -27,6 +27,6 @@ static inline int f(int k) {
#include "macro_arg_directive.h" // expected-error {{embedding a #include directive within macro arguments is not supported}}
-int g(int k) {
+int g(int k) { // expected-error {{expected ')'}}
return f(k) + f(k-1));
}
diff --git a/clang/test/Preprocessor/print_line_track.c b/clang/test/Preprocessor/print_line_track.c
index 156ae22693b85..7df02c524a113 100644
--- a/clang/test/Preprocessor/print_line_track.c
+++ b/clang/test/Preprocessor/print_line_track.c
@@ -1,9 +1,9 @@
-/* RUN: %clang_cc1 -E %s | grep 'a 3'
- * RUN: %clang_cc1 -E %s | grep 'b 16'
- * RUN: %clang_cc1 -E -P %s | grep 'a 3'
- * RUN: %clang_cc1 -E -P %s | grep 'b 16'
+/* RUN: %clang_cc1 -E %s | awk '/a/,/3/{exit 0} {exit 1}'
+ * RUN: %clang_cc1 -E %s | awk '/b/,/16/{exit 0} {exit 1}'
+ * RUN: %clang_cc1 -E -P %s | awk '/a/,/3/{exit 0} {exit 1}'
+ * RUN: %clang_cc1 -E -P %s | awk '/b/,/16/{exit 0} {exit 1}'
* RUN: %clang_cc1 -E %s | not grep '# 0 '
- * RUN: %clang_cc1 -E -P %s | count 2
+ * RUN: %clang_cc1 -E -P %s | count 4
* PR1848 PR3437 PR7360
*/
@@ -14,4 +14,3 @@ t(a
t(b
__LINE__)
-
|
Member
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Sergej Salnikov (SergejSalnikov) ChangesNow the files location is used for macro expansions. This provides more accurate location when reporting compilation errors. Move from (it's a rollforward of #166255 with fixed test) Full diff: https://github.com/llvm/llvm-project/pull/168402.diff 8 Files Affected:
diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
index bc9e97863556d..f15257a760b8c 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -1464,8 +1464,9 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// directives. This provides a view on the data that a user should see
/// in diagnostics, for example.
///
- /// Note that a presumed location is always given as the expansion point of
- /// an expansion location, not at the spelling location.
+ /// If \p Loc is a macro expansion location, the presumed location
+ /// computation uses the spelling location for macro arguments and the
+ /// expansion location for other macro expansions.
///
/// \returns The presumed location of the specified SourceLocation. If the
/// presumed location cannot be calculated (e.g., because \p Loc is invalid
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index b6cc6ec9365f5..767a765ae4261 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -1435,7 +1435,7 @@ PresumedLoc SourceManager::getPresumedLoc(SourceLocation Loc,
if (Loc.isInvalid()) return PresumedLoc();
// Presumed locations are always for expansion points.
- FileIDAndOffset LocInfo = getDecomposedExpansionLoc(Loc);
+ FileIDAndOffset LocInfo = getDecomposedLoc(getFileLoc(Loc));
bool Invalid = false;
const SLocEntry &Entry = getSLocEntry(LocInfo.first, &Invalid);
diff --git a/clang/test/Analysis/plist-macros-with-expansion.cpp b/clang/test/Analysis/plist-macros-with-expansion.cpp
index d57bb0f2dd265..d9a2f94055593 100644
--- a/clang/test/Analysis/plist-macros-with-expansion.cpp
+++ b/clang/test/Analysis/plist-macros-with-expansion.cpp
@@ -405,14 +405,14 @@ void commaInBracketsTest() {
code
void commaInBracesTest() {
- PASTE_CODE({ // expected-warning{{Dereference of null pointer}}
+ PASTE_CODE({
// NOTE: If we were to add a new variable here after a comma, we'd get a
// compilation error, so this test is mainly here to show that this was also
// investigated.
//
// int *ptr = nullptr, a;
int *ptr = nullptr;
- *ptr = 5;
+ *ptr = 5; // expected-warning{{Dereference of null pointer}}
})
}
@@ -425,14 +425,14 @@ void commaInBracesTest() {
// CHECK-NEXT: <key>col</key><integer>3</integer>
// CHECK-NEXT: <key>file</key><integer>0</integer>
// CHECK-NEXT: </dict>
-// CHECK-NEXT: <key>name</key><string>PASTE_CODE({ // expected-
+// CHECK-NEXT: <key>name</key><string>PASTE_CODE({
// CHECK-NEXT: // NOTE: If we were to add a new variable here after a comma, we'd get a
// CHECK-NEXT: // compilation error, so this test is mainly here to show that this was also
// CHECK-NEXT: // investigated.
// CHECK-NEXT: //
// CHECK-NEXT: // int *ptr = nullptr, a;
// CHECK-NEXT: int *ptr = nullptr;
-// CHECK-NEXT: *ptr = 5;
+// CHECK-NEXT: *ptr = 5; // expected-
// CHECK-NEXT: })</string>
// CHECK-NEXT: <key>expansion</key><string>{int *ptr =nullptr ;*ptr =5;}</string>
// CHECK-NEXT: </dict>
diff --git a/clang/test/C/C23/n2350.c b/clang/test/C/C23/n2350.c
index af0ca6d79be5e..96b8c511d5716 100644
--- a/clang/test/C/C23/n2350.c
+++ b/clang/test/C/C23/n2350.c
@@ -47,11 +47,10 @@ int struct_in_second_param(void) {
int macro(void) {
return offsetof(struct A // cpp-error {{'A' cannot be defined in a type specifier}} \
- expected-warning 2 {{defining a type within 'offsetof' is a C23 extension}}
+ expected-warning {{defining a type within 'offsetof' is a C23 extension}}
{
int a;
- struct B // verifier seems to think the error is emitted by the macro
- // In fact the location of the error is "B" on the line above
+ struct B // expected-warning {{defining a type within 'offsetof' is a C23 extension}}
{
int c;
int d;
diff --git a/clang/test/ExtractAPI/macro_undefined.c b/clang/test/ExtractAPI/macro_undefined.c
index 7bb50af380c24..1d697db1e1613 100644
--- a/clang/test/ExtractAPI/macro_undefined.c
+++ b/clang/test/ExtractAPI/macro_undefined.c
@@ -89,7 +89,7 @@ FUNC_GEN(bar, const int *, unsigned);
},
"location": {
"position": {
- "character": 0,
+ "character": 9,
"line": 2
},
"uri": "file://INPUT_DIR/input.h"
@@ -241,7 +241,7 @@ FUNC_GEN(bar, const int *, unsigned);
},
"location": {
"position": {
- "character": 0,
+ "character": 9,
"line": 3
},
"uri": "file://INPUT_DIR/input.h"
diff --git a/clang/test/FixIt/format.cpp b/clang/test/FixIt/format.cpp
index d663c0fb35e13..db642b60ffd95 100644
--- a/clang/test/FixIt/format.cpp
+++ b/clang/test/FixIt/format.cpp
@@ -56,9 +56,9 @@ void a(N::E NEVal, S *SPtr, S &SRef) {
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:17}:")"
- LOG( // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
+ LOG(
"%d",
- SPtr->Type
+ SPtr->Type // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
);
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:17}:")"
@@ -68,8 +68,8 @@ void a(N::E NEVal, S *SPtr, S &SRef) {
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:")"
- LOG("%d", // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
- SRef.Type);
+ LOG("%d",
+ SRef.Type); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:")"
diff --git a/clang/test/Preprocessor/macro_arg_directive.c b/clang/test/Preprocessor/macro_arg_directive.c
index 929a03d70d025..c612aa545a2a9 100644
--- a/clang/test/Preprocessor/macro_arg_directive.c
+++ b/clang/test/Preprocessor/macro_arg_directive.c
@@ -18,7 +18,7 @@ void fail(const char *);
({ int result = 0; __VA_ARGS__; if (!result) { fail(#__VA_ARGS__); }; result })
static inline int f(int k) {
- return MUNCH( // expected-error {{expected ')'}} expected-note {{to match this '('}} expected-error {{returning 'void'}} expected-note {{expansion of macro 'MUNCH' requested here}}
+ return MUNCH( // expected-note {{to match this '('}} expected-error {{returning 'void'}} expected-note {{expansion of macro 'MUNCH' requested here}}
if (k < 3)
result = 24;
else if (k > 4)
@@ -27,6 +27,6 @@ static inline int f(int k) {
#include "macro_arg_directive.h" // expected-error {{embedding a #include directive within macro arguments is not supported}}
-int g(int k) {
+int g(int k) { // expected-error {{expected ')'}}
return f(k) + f(k-1));
}
diff --git a/clang/test/Preprocessor/print_line_track.c b/clang/test/Preprocessor/print_line_track.c
index 156ae22693b85..7df02c524a113 100644
--- a/clang/test/Preprocessor/print_line_track.c
+++ b/clang/test/Preprocessor/print_line_track.c
@@ -1,9 +1,9 @@
-/* RUN: %clang_cc1 -E %s | grep 'a 3'
- * RUN: %clang_cc1 -E %s | grep 'b 16'
- * RUN: %clang_cc1 -E -P %s | grep 'a 3'
- * RUN: %clang_cc1 -E -P %s | grep 'b 16'
+/* RUN: %clang_cc1 -E %s | awk '/a/,/3/{exit 0} {exit 1}'
+ * RUN: %clang_cc1 -E %s | awk '/b/,/16/{exit 0} {exit 1}'
+ * RUN: %clang_cc1 -E -P %s | awk '/a/,/3/{exit 0} {exit 1}'
+ * RUN: %clang_cc1 -E -P %s | awk '/b/,/16/{exit 0} {exit 1}'
* RUN: %clang_cc1 -E %s | not grep '# 0 '
- * RUN: %clang_cc1 -E -P %s | count 2
+ * RUN: %clang_cc1 -E -P %s | count 4
* PR1848 PR3437 PR7360
*/
@@ -14,4 +14,3 @@ t(a
t(b
__LINE__)
-
|
Contributor
Author
|
#AaronBallman, I've created another PR with a fix. Could you please run the failed targets to verify that the fixed version works? |
Collaborator
|
Precommit CI found some relevant failures; but I'm not certain |
🐧 Linux x64 Test Results
|
| t(b | ||
| __LINE__) | ||
| t( | ||
| a 3) |
Collaborator
There was a problem hiding this comment.
Can you help me understand why adding whitespace here will repair the test failure?
Contributor
Author
There was a problem hiding this comment.
the problem is that change impact the output of -E command. It used to be that macro was expanded by preprocessor on a single line, but now the macro argument preserves the line numbers, so a 3 used to be rendered on a single line, now that will be 2 lines. (and grep doesn't match well multiline pattern).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now the files location is used for macro expansions. This provides more accurate location when reporting compilation errors.
Move from
getDecomposedExpansionLoc(Loc)togetDecomposedLoc(getFileLoc(Loc))when computing Presumed location.(it's a rollforward of #166255 with fixed clang/test/Preprocessor/print_line_track.c test)