Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LLD] [COFF] Clarify -print-search-path for the empty string element #67856

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

mstorsjo
Copy link
Member

Also switch the test case to use -NEXT to strictly match all lines.

Also switch the test case to use -NEXT to strictly match all lines.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 29, 2023

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-lld-coff

@llvm/pr-subscribers-platform-windows

Changes

Also switch the test case to use -NEXT to strictly match all lines.


Full diff: https://github.com/llvm/llvm-project/pull/67856.diff

2 Files Affected:

  • (modified) lld/COFF/Driver.cpp (+4-1)
  • (modified) lld/test/COFF/print-search-paths.s (+18-14)
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 61a04a74aa60278..65309168440add2 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -2084,8 +2084,11 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
     raw_svector_ostream stream(buffer);
     stream << "Library search paths:\n";
 
-    for (StringRef path : searchPaths)
+    for (StringRef path : searchPaths) {
+      if (path == "")
+        path = "(cwd)";
       stream << "  " << path << "\n";
+    }
 
     message(buffer);
   }
diff --git a/lld/test/COFF/print-search-paths.s b/lld/test/COFF/print-search-paths.s
index 5c292aafc68a11c..a8c8047fd74ae38 100644
--- a/lld/test/COFF/print-search-paths.s
+++ b/lld/test/COFF/print-search-paths.s
@@ -4,21 +4,25 @@
 # RUN: llvm-mc -triple i686-windows-msvc %s -filetype=obj -o %t_32.obj
 # RUN: lld-link -safeseh:no /dll /noentry /winsysroot:%t.dir/sysroot /vctoolsversion:1.1.1.1 /winsdkversion:10.0.1 %t_32.obj -print-search-paths | FileCheck -DSYSROOT=%t.dir -check-prefix=X86 %s
 # CHECK: Library search paths:
-# CHECK:   [[CPATH:.*]]lib{{[/\\]}}clang{{[/\\]}}{{[0-9]+}}{{[/\\]}}lib{{[/\\]}}windows
-# CHECK:   [[CPATH]]lib{{[/\\]}}clang{{[/\\]}}{{[0-9]+}}{{[/\\]}}lib
-# CHECK:   [[CPATH]]lib
-# CHECK:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}VC{{[/\\]}}Tools{{[/\\]}}MSVC{{[/\\]}}1.1.1.1{{[/\\]}}lib{{[/\\]}}x64
-# CHECK:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}VC{{[/\\]}}Tools{{[/\\]}}MSVC{{[/\\]}}1.1.1.1{{[/\\]}}atlmfc{{[/\\]}}lib{{[/\\]}}x64
-# CHECK:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}Windows Kits{{[/\\]}}10{{[/\\]}}Lib{{[/\\]}}10.0.1{{[/\\]}}ucrt{{[/\\]}}x64
-# CHECK:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}Windows Kits{{[/\\]}}10{{[/\\]}}Lib{{[/\\]}}10.0.1{{[/\\]}}um{{[/\\]}}x64
+# CHECK-NEXT:   [[CPATH:.*]]lib{{[/\\]}}clang{{[/\\]}}{{[0-9]+}}{{[/\\]}}lib{{[/\\]}}windows
+# CHECK-NEXT:   [[CPATH]]lib{{[/\\]}}clang{{[/\\]}}{{[0-9]+}}{{[/\\]}}lib
+# CHECK-NEXT:   [[CPATH]]lib
+# CHECK-NEXT:   (cwd)
+# CHECK-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}DIA SDK{{[/\\]}}lib{{[/\\]}}amd64
+# CHECK-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}VC{{[/\\]}}Tools{{[/\\]}}MSVC{{[/\\]}}1.1.1.1{{[/\\]}}lib{{[/\\]}}x64
+# CHECK-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}VC{{[/\\]}}Tools{{[/\\]}}MSVC{{[/\\]}}1.1.1.1{{[/\\]}}atlmfc{{[/\\]}}lib{{[/\\]}}x64
+# CHECK-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}Windows Kits{{[/\\]}}10{{[/\\]}}Lib{{[/\\]}}10.0.1{{[/\\]}}ucrt{{[/\\]}}x64
+# CHECK-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}Windows Kits{{[/\\]}}10{{[/\\]}}Lib{{[/\\]}}10.0.1{{[/\\]}}um{{[/\\]}}x64
 # X86: Library search paths:
-# X86:   [[CPATH:.*]]lib{{[/\\]}}clang{{[/\\]}}{{[0-9]+}}{{[/\\]}}lib{{[/\\]}}windows
-# X86:   [[CPATH]]lib{{[/\\]}}clang{{[/\\]}}{{[0-9]+}}{{[/\\]}}lib
-# X86:   [[CPATH]]lib
-# X86:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}VC{{[/\\]}}Tools{{[/\\]}}MSVC{{[/\\]}}1.1.1.1{{[/\\]}}lib{{[/\\]}}x86
-# X86:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}VC{{[/\\]}}Tools{{[/\\]}}MSVC{{[/\\]}}1.1.1.1{{[/\\]}}atlmfc{{[/\\]}}lib{{[/\\]}}x86
-# X86:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}Windows Kits{{[/\\]}}10{{[/\\]}}Lib{{[/\\]}}10.0.1{{[/\\]}}ucrt{{[/\\]}}x86
-# X86:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}Windows Kits{{[/\\]}}10{{[/\\]}}Lib{{[/\\]}}10.0.1{{[/\\]}}um{{[/\\]}}x86
+# X86-NEXT:   [[CPATH:.*]]lib{{[/\\]}}clang{{[/\\]}}{{[0-9]+}}{{[/\\]}}lib{{[/\\]}}windows
+# X86-NEXT:   [[CPATH]]lib{{[/\\]}}clang{{[/\\]}}{{[0-9]+}}{{[/\\]}}lib
+# X86-NEXT:   [[CPATH]]lib
+# X86-NEXT:   (cwd)
+# X86-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}DIA SDK{{[/\\]}}lib
+# X86-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}VC{{[/\\]}}Tools{{[/\\]}}MSVC{{[/\\]}}1.1.1.1{{[/\\]}}lib{{[/\\]}}x86
+# X86-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}VC{{[/\\]}}Tools{{[/\\]}}MSVC{{[/\\]}}1.1.1.1{{[/\\]}}atlmfc{{[/\\]}}lib{{[/\\]}}x86
+# X86-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}Windows Kits{{[/\\]}}10{{[/\\]}}Lib{{[/\\]}}10.0.1{{[/\\]}}ucrt{{[/\\]}}x86
+# X86-NEXT:   [[SYSROOT]]{{[/\\]}}sysroot{{[/\\]}}Windows Kits{{[/\\]}}10{{[/\\]}}Lib{{[/\\]}}10.0.1{{[/\\]}}um{{[/\\]}}x86
 
 
         .text

Copy link
Collaborator

@tru tru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I wonder about the (cwd) - couldn't we print the actual cwd in that case?

@mstorsjo
Copy link
Member Author

mstorsjo commented Oct 2, 2023

Looks good, but I wonder about the (cwd) - couldn't we print the actual cwd in that case?

I guess we could, although that's a bit messier with respect to tests. (Also, perhaps it kinda loses a little bit of distinction between whether the path actually was specified as an absolute path or not - which probably isn't important?)

In all honesty, it was just the quickest solution I had for making this clearer in tests; I also considered wrapping the path in quotes, which makes it clear that we have one entry with an empty path. But Clang's printing of include directories is a similar feature, which doesn't quote the paths, so I felt it was better to go with consistency with that.

Copy link
Collaborator

@zmodem zmodem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@tru
Copy link
Collaborator

tru commented Oct 2, 2023

Looks good, but I wonder about the (cwd) - couldn't we print the actual cwd in that case?

I guess we could, although that's a bit messier with respect to tests. (Also, perhaps it kinda loses a little bit of distinction between whether the path actually was specified as an absolute path or not - which probably isn't important?)

In all honesty, it was just the quickest solution I had for making this clearer in tests; I also considered wrapping the path in quotes, which makes it clear that we have one entry with an empty path. But Clang's printing of include directories is a similar feature, which doesn't quote the paths, so I felt it was better to go with consistency with that.

I am fine with this!

@mstorsjo
Copy link
Member Author

mstorsjo commented Oct 2, 2023

Looks good, but I wonder about the (cwd) - couldn't we print the actual cwd in that case?

I guess we could, although that's a bit messier with respect to tests. (Also, perhaps it kinda loses a little bit of distinction between whether the path actually was specified as an absolute path or not - which probably isn't important?)
In all honesty, it was just the quickest solution I had for making this clearer in tests; I also considered wrapping the path in quotes, which makes it clear that we have one entry with an empty path. But Clang's printing of include directories is a similar feature, which doesn't quote the paths, so I felt it was better to go with consistency with that.

I am fine with this!

Ok, great, thanks! I also checked, and it doesn't look like lit has got a substitution for the cwd similar to how it has got %t and %S and such.

@mstorsjo mstorsjo merged commit 7d7d9e4 into llvm:main Oct 2, 2023
5 of 6 checks passed
@mstorsjo mstorsjo deleted the lld-print-search-path-cwd branch October 2, 2023 10:59
tru pushed a commit that referenced this pull request Oct 2, 2023
…67856)

Also switch the test case to use -NEXT to strictly match all lines.

(cherry picked from commit 7d7d9e4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants