[clang] [Clang] [Driver] Canoncalise `-internal-isystem` include paths (PR #148745)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> > Sure, but I was thinking more that we do want to canonicalize paths like `blah\src/foo.cc` to `blah\src\foo.cc` on Windows. so there are consistent Windows path separators being used. Perhaps my use of "canonicalize" isn't right. :-)
> 
> Yeah, that would be nice, but canonicalisation afaik always involves conversion to an absolute path; I don’t know if we have a function that normalises paths like that.
> 
> > Agreed; I mostly was suggesting that whatever path ends up being shorter, it would still be nice to ensure the path printed out uses consistent separators (so the length stays the same, it's just a more natural-looking way to express the file for that particular file system).
> 
> I honestly do wonder if it wouldn’t make more sense to go back to doing this in `SourceManager`—or somewhere in the diagnostics machinery; what with all of the tests that are affected by this, the fact that we probably want a separate flag for this anyway, and the fact that it’s a bit ‘non-deterministic’ in the sense that I don’t think we really care what exaclty we end up printing so long as it’s 1. a valid path, and 2. no larger than the path we started with (and I suppose 3. making path separators more consistent would also be nice).
> 
> I feel like this change is really only relevant for the very specific situation of printing diagnostics to the terminal

I was thinking it's also useful for `-###` and `-v` output, crash reports, `-ast-dump` output...

> —an IDE can just point you to the error directly, and e.g. SARIF isn’t user-facing either way so I don’t think you’d care about simplified paths in either situation. 

I think SARIF already has to care, somewhat. IIRC the paths in SARIF files have to follow a particular RFC (RFC 3986 I think?) and that will dictate how paths are referenced.

> With how this has turned out, doing this (close to) where we print the diagnostics to me seems like it would be less of a hassle...
> 
> Either way though, I think we do need a separate flag that enables/disables this.

Yeah, I think the flag will be necessary, though hopefully we can default the flag to enable it.

https://github.com/llvm/llvm-project/pull/148745