Respect "-fdiagnostics-absolute-paths" on emit include location

[charmitro]

Summary

This commit fixes "TextDiagnostic::emitIncludeLocation" when compiling
with "-fdiagnostics-absolute-paths" flag enabled by emitting the absolute
path of the included file.

Fixes #63026

[tbaeder]

Can you add a test case for the change? Looks like there's something similar already in clang/test/Frontend/absolute-paths.c

[charmitro]

In D151833#4386128, @tbaeder wrote:

Can you add a test case for the change? Looks like there's something similar already in clang/test/Frontend/absolute-paths.c

Is it possible to use substitutions inside let's say NORMAL: ? How could I resolve the absolute path without hard-coding my path inside the test case?

[charmitro]

In D151833#4390279, @charmitro wrote:

In D151833#4386128, @tbaeder wrote:

Can you add a test case for the change? Looks like there's something similar already in clang/test/Frontend/absolute-paths.c

Is it possible to use substitutions inside let's say NORMAL: ? How could I resolve the absolute path without hard-coding my path inside the test case?

I was able to make it work using -DNAME=<value> in the FileCheck invocation inside absolute-paths.c test file.

[tbaeder]

clang/test/Frontend/absolute-paths.c

6 ↗

(On Diff #527828)

This checks the same thing in both cases, but in the NORMAL case, it should not use the absolute path, shouldn't it?

[charmitro]

clang/test/Frontend/absolute-paths.c

6 ↗

(On Diff #527828)

You're correct. I was constantly testing it from the same path.

Since the relative path is going to be different depending on where you running from, would it be wise to accept a regular expression of any string in the NORMAL case?

For example,

NORMAL: In file included from {{.*}}:

[tbaeder]

clang/test/Frontend/absolute-paths.c

6 ↗

(On Diff #527828)

I wonder if it would make sense to just check for the filename in the NORMAL case and then do a NORMAL-NOT: [[ROOT_ABSOLUTE]]?

[charmitro]

clang/test/Frontend/absolute-paths.c

6 ↗

(On Diff #527828)

Yes, that way we are testing if the warning contains the correct filename+LOC.

Something like: // NORMAL: In file included from {{.*absolute-paths.c:4}}:.

But why changefrom ABSOLUTE: to NORMAL-NOT?

[tbaeder]

clang/test/Frontend/absolute-paths.c

6 ↗

(On Diff #527828)

Your regex checks if the path ends with the file name, right? That would be true in both cases.

[charmitro]

clang/test/Frontend/absolute-paths.c

6 ↗

(On Diff #527828)

What else do you propose here?

Let me explain why this was the only obvious option for me.

In the case of NORMAL, it checks for the relative path based on the location you invoke the test commands.

Given that, if we, for example,

1. run llvm-lit from the project root, the NORMAL case expects:

In file included from clang/test/Frontend/absolute-paths.c:4:

2. run llvm-lit from $PROJECT_ROOT/clang/test/, the NORMAL case expects:

In file included from Frontend/absolute-paths.c:4:

It seems unwise to me to hard-code the path based on what the CI excepts in the NORMAL scenario, because that way, whoever runs the test cases manually, will experience test failure.

[tbaeder]

clang/test/Frontend/absolute-paths.c

6 ↗

(On Diff #527828)

You can still keep the ABSOLUTE: [[ROOT_ABSOLUTE]] stuff, I was just proposing to add the NORMAL-NOT one additionally, so we can make sure we're not printing the absolute path in the normal case as well.

[tbaeder]

LGTM, but maybe wait for a response from @aaron.ballman or @cjdb

[aaron.ballman]

LGTM!

[aaron.ballman]

Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

[tbaeder]

@charmitro do you have commit rights or do you need someone to push on your behalf?

[charmitro]

In D151833#4395956, @aaron.ballman wrote:

Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

In D151833#4395957, @tbaeder wrote:

@charmitro do you have commit rights or do you need someone to push on your behalf?

I do not have commit rights, since I'm planning to contribute more often, I'd like commit access at some point. I can request it ASAP if needed.

[cjdb]

Can the commit message have a description please? It's unclear to me why this is necessary (although I'm sure there's a good reason).

[charmitro]

In D151833#4396244, @cjdb wrote:

Can the commit message have a description please? It's unclear to me why this is necessary (although I'm sure there's a good reason).

Thanks! Done, also check https://github.com/llvm/llvm-project/issues/63026

[cjdb]

In D151833#4396399, @charmitro wrote:

In D151833#4396244, @cjdb wrote:

Can the commit message have a description please? It's unclear to me why this is necessary (although I'm sure there's a good reason).

Thanks! Done, also check https://github.com/llvm/llvm-project/issues/63026

Nice, thanks! Would you mind adding Fixes #63026. somewhere in the commit message? That will auto-close the issue upon merge.

[charmitro]

In D151833#4396426, @cjdb wrote:

In D151833#4396399, @charmitro wrote:

In D151833#4396244, @cjdb wrote:

Can the commit message have a description please? It's unclear to me why this is necessary (although I'm sure there's a good reason).

Thanks! Done, also check https://github.com/llvm/llvm-project/issues/63026

Nice, thanks! Would you mind adding Fixes #63026. somewhere in the commit message? That will auto-close the issue upon merge.

Thanks for pointing that, done.

[charmitro]

In D151833#4396008, @charmitro wrote:

In D151833#4395956, @aaron.ballman wrote:

Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

In D151833#4395957, @tbaeder wrote:

@charmitro do you have commit rights or do you need someone to push on your behalf?

I do not have commit rights, since I'm planning to contribute more often, I'd like commit access at some point. I can request it ASAP if needed.

Author should be:
Charalampos Mitrodimas <[e-mail]>

[cjdb]

I already have another commit I plan to push, will do these together :)

clang/lib/Frontend/TextDiagnostic.cpp

if (DiagOpts->ShowSourceRanges && !Ranges.empty()) {
 
     if (PrintedRange)
       OS << ':';
   }
   OS << ' ';
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-    OS << "In file included from " << PLoc.getFilename() << ':'
-       << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+    OS << "In file included from ";
+    emitFilename(PLoc.getFilename(), Loc.getManager());
+    OS << ':' << PLoc.getLine() << ":\n";
+  } else
     OS << "In included file:\n";
 }
 
 void TextDiagnostic::emitImportLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                                         StringRef ModuleName) {
   if (DiagOpts->ShowLocation && PLoc.isValid())
     OS << "In module '" << ModuleName << "' imported from "
        << PLoc.getFilename() << ':' << PLoc.getLine() << ":\n";