[PATCH] D85024: [llvm-objdump] Implement --prefix option
Vinicius Tinti via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 24 17:57:18 PDT 2020
tinti added inline comments.
================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:15
+
+; RUN: llvm-objdump --prefix %p --source %t.o 2>&1 | FileCheck %s --check-prefix CHECK-MISS-PREFIX-FIX
+; CHECK-MISS-PREFIX-FIX: ; int foo() {
----------------
tinti wrote:
> jhenderson wrote:
> > Hmmm... this is interesting. With llvm-objdump, this test case fails for Windows. However, if I replace llvm-objdump with GNU objdump and run the same invocation, it works. I dug into this a little. The problem is with the meaning of "absolute paths" for Windows - it's ambiguous. C++17's `filesystem::is_absolute`, whose behaviour is matched by `sys::path::is_absolute` essentially says that the path must be unambiguous (i.e. that there must be no drive letter for Windows). However, other languages, e.g. Python, treat a path as absolute with or without the drive letter. GNU objdump appears to follow the Python etc approach, so '/Inputs\foo' is treated as an absolute path and is prefixed.
> >
> > I think we need extra logic to get this to work on Windows, to ensure GNU compatibility. Basically, you need to identify the case where a path is absolute within the current drive (i.e. starts with '/', though it might be better, if possible to rely on the underlying functions to do that), and treat that as absolute. What you can't do is modify the behaviour of `sys::path::is_absolute` itself.
> I did a small test.
>
> ```
> bool is_absolute2(const llvm::Twine &path, llvm::sys::path::Style style) {
> if (style == llvm::sys::path::Style::windows) {
> llvm::SmallString<128> path_storage;
> llvm::StringRef p = path.toStringRef(path_storage);
>
> if (p.front() == '/' || p.front() == '\\')
> return true;
>
> if (p.size() >= 2 && (p[0] && p[1] == ':'))
> return true;
> }
>
> return llvm::sys::path::is_absolute(path, style);
> }
>
> bool is_separator2(char value, llvm::sys::path::Style style) {
> // not needed
> return llvm::sys::path::is_separator(value, style);
> }
>
> ...
>
> std::string test1("/input/foo.c");
> std::string test2("c:/input/foo.c");
> std::string test3("\\\\input\\foo.c");
> std::string test4("c:/");
> std::string test5("c:");
> std::string test6("//");
> std::string test7("\\");
> std::string test8(":");
>
> std::vector<std::string> tests;
> tests.push_back(test1);
> tests.push_back(test2);
> tests.push_back(test3);
> tests.push_back(test4);
> tests.push_back(test5);
> tests.push_back(test6);
> tests.push_back(test7);
> tests.push_back(test8);
>
> for (auto it = tests.begin(); it < tests.end(); ++it) {
> llvm::outs() << "is_absolute?(\"" << *it << "\")" << "\n";
> llvm::outs() << " gnu::windows: " << IS_DOS_ABSOLUTE_PATH(it->c_str()) << "\n";
> llvm::outs() << " llvm::windows: " << llvm::sys::path::is_absolute(*it, llvm::sys::path::Style::windows) << "\n";
> llvm::outs() << " llvm2::windows: " << is_absolute2(*it, llvm::sys::path::Style::windows) << "\n";
>
> llvm::outs() << " gnu::posix: " << IS_UNIX_ABSOLUTE_PATH(it->c_str()) << "\n";
> llvm::outs() << " llvm::posix: " << llvm::sys::path::is_absolute(*it, llvm::sys::path::Style::posix) << "\n";
> llvm::outs() << " llvm2::posix: " << is_absolute2(*it, llvm::sys::path::Style::posix) << "\n";
>
> llvm::outs() << "is_separator?(\"" << *it << "\")" << "\n";
> llvm::outs() << " gnu::windows: " << IS_DOS_DIR_SEPARATOR(it->back()) << "\n";
> llvm::outs() << " llvm::windows: " << llvm::sys::path::is_separator(it->back(), llvm::sys::path::Style::windows) << "\n";
> llvm::outs() << " llvm2::windows: " << is_separator2(it->back(), llvm::sys::path::Style::windows) << "\n";
>
> llvm::outs() << " gnu::posix: " << IS_UNIX_DIR_SEPARATOR(it->back()) << "\n";
> llvm::outs() << " llvm::posix: " << llvm::sys::path::is_separator(it->back(), llvm::sys::path::Style::posix) << "\n";
> llvm::outs() << " llvm2::posix: " << is_separator2(it->back(), llvm::sys::path::Style::posix) << "\n";
> llvm::outs() << "\n";
> }
> ```
>
> The output is:
>
> ```
> is_absolute?("/input/foo.c")
> gnu::windows: 1
> llvm::windows: 0
> llvm2::windows: 1
> gnu::posix: 1
> llvm::posix: 1
> llvm2::posix: 1
> is_separator?("/input/foo.c")
> gnu::windows: 0
> llvm::windows: 0
> llvm2::windows: 0
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
>
> is_absolute?("c:/input/foo.c")
> gnu::windows: 1
> llvm::windows: 1
> llvm2::windows: 1
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
> is_separator?("c:/input/foo.c")
> gnu::windows: 0
> llvm::windows: 0
> llvm2::windows: 0
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
>
> is_absolute?("\\input\foo.c")
> gnu::windows: 1
> llvm::windows: 1
> llvm2::windows: 1
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
> is_separator?("\\input\foo.c")
> gnu::windows: 0
> llvm::windows: 0
> llvm2::windows: 0
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
>
> is_absolute?("c:/")
> gnu::windows: 1
> llvm::windows: 1
> llvm2::windows: 1
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
> is_separator?("c:/")
> gnu::windows: 1
> llvm::windows: 1
> llvm2::windows: 1
> gnu::posix: 1
> llvm::posix: 1
> llvm2::posix: 1
>
> is_absolute?("c:")
> gnu::windows: 1
> llvm::windows: 0
> llvm2::windows: 1
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
> is_separator?("c:")
> gnu::windows: 0
> llvm::windows: 0
> llvm2::windows: 0
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
>
> is_absolute?("//")
> gnu::windows: 1
> llvm::windows: 0
> llvm2::windows: 1
> gnu::posix: 1
> llvm::posix: 1
> llvm2::posix: 1
> is_separator?("//")
> gnu::windows: 1
> llvm::windows: 1
> llvm2::windows: 1
> gnu::posix: 1
> llvm::posix: 1
> llvm2::posix: 1
>
> is_absolute?("\")
> gnu::windows: 1
> llvm::windows: 0
> llvm2::windows: 1
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
> is_separator?("\")
> gnu::windows: 1
> llvm::windows: 1
> llvm2::windows: 1
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
>
> is_absolute?(":")
> gnu::windows: 0
> llvm::windows: 0
> llvm2::windows: 0
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
> is_separator?(":")
> gnu::windows: 0
> llvm::windows: 0
> llvm2::windows: 0
> gnu::posix: 0
> llvm::posix: 0
> llvm2::posix: 0
>
>
> ```
>
> GNU and LLVM don't agree with `/input/foo.c`, `c:`, `/` and `\`. The `is_absolute2` function agrees with GNU. I believe it is enough for this patch.
>
> Working on it!
I believe this time it will work. I have added a helper function `bool objdumpIsAbsolute(const Twine &path, sys::path::Style style = sys::path::Style::native);` at `llvm-objdump.cpp`. At first I have added it on `llvm-objdump.h` but I changed now. Should I keep it this way?
About Windows, would you mind to test again?
================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:25
+
+; Test remove all trailing separators (i.e. '/') in prefix.
+; The prefix '/' is the same as not using the `--prefix` option.
----------------
jhenderson wrote:
>
Ok.
At least it appears that GNU's and LLVM's objdump agree on `is_separator`.
================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:28-29
+
+; RUN: llvm-objdump --prefix / --source %t2.o 2>&1 | FileCheck %s --check-prefix CHECK-TRAILING1
+; CHECK-TRAILING1: ; int foo() {
+
----------------
jhenderson wrote:
> You can avoid the duplicate check pattern here, since the output should be the same as earlier.
I think I understand now what you were trying to say in the last replies.
I have changed to `CHECK-MISS-PREFIX-FIX` which produces the same output as the removed `CHECK-TRAILING1`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85024/new/
https://reviews.llvm.org/D85024
More information about the llvm-commits
mailing list