[PATCH] D87732: [Support] Provide sys::path::guess_style

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

In D87732#2275529 <https://reviews.llvm.org/D87732#2275529>, @dblaikie wrote:

> Hmm - probably simpler to justify this patch by including refactoring at least one (possibly more) existing uses you mentioned, to use this API - then I probably don't really need to understand what it's doing, just that it's generalizing some existing functionality.

Done.

> But if I am going to understand what this is doing - could you explain in a bit more detail (perhaps with examples) what went wrong with the other review/patch and how this'll address it? I wasn't quite able to follow the patch description, sorry :/

The problem is that `GetFileByIndex` has been using `llvm::sys::path::native` as the separator style which is the separator of the platform we're running on, but not the platform where the debug info was generated. So for example, say I produce an ELF file with debug info on Linux, but then try to use `llvm-dwarfdump` with that file on Windows. Today, I'd get `/home/username/source\file.c` as the path, assuming `/home/username/source` being `comp_dir` and `file.c` being the filename. That's because `GetFileByIndex` uses `sys::path::append` with `llvm::sys::path::native` as a style which on Windows would result in `\` being used as separator instead of `/`. That's why tests like https://github.com/llvm/llvm-project/blob/c913f6dce69513b430f705d5a1f4e745f5d0a27e/lld/test/wasm/debuginfo.test#L19 use regex to match both separators for the last component. With D87657 <https://reviews.llvm.org/D87657> this becomes worse because `llvm::sys::path:remove_dots` rewrites all separators as part of normalization, so you end up with `/home\username\source\file.c`. Rather than updating all tests to accepts both separators for all components, I made this change which tries to infer the correct separator which I think is a better solution overall: if I have a file produced on Linux, even if I'm reading it on Windows, I'd still expect `/` to be used as path separators. Does that make sense?

Potential alternative would be to always use `llvm::sys::path::posix` everywhere since on modern Windows, all syscalls and tools should accept `/` as a path separator, but I'm worried that's going to be a more intrusive change.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87732/new/

https://reviews.llvm.org/D87732