subject:"\[PATCH\] D109828\: \[clang\-cl\] Add a \/diasdkdir flag and make \/winsysroot imply it"

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

2021-09-16 Thread Nico Weber via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG951f362e2560: [clang-cl] Add a /diasdkdir flag and make 
/winsysroot imply it (authored by thakis).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D109828?vs=372782=372898#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109828

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/test/Driver/cl-sysroot.cpp

Index: clang/test/Driver/cl-sysroot.cpp
===
--- clang/test/Driver/cl-sysroot.cpp
+++ clang/test/Driver/cl-sysroot.cpp
@@ -1,18 +1,27 @@
 // RUN: rm -rf %t
 // RUN: split-file %s %t
 
-// RUN: %clang_cl /winsysroot %t -### -- %t/foo.cpp 2>&1 | FileCheck %s
-// RUN: %clang_cl /vctoolsdir %t/VC/Tools/MSVC/27.1828.18284 \
-// RUN:   /winsdkdir "%t/Windows Kits/10" \
-// RUN:   -### -- %t/foo.cpp 2>&1 | FileCheck %s
+// RUN: %clang_cl -m64 /winsysroot %t -### -- %t/foo.cpp 2>&1 | FileCheck %s
+// RUN: %clang_cl -m64 \
+// RUN: /diasdkdir "%t/DIA SDK" \
+// RUN: /vctoolsdir %t/VC/Tools/MSVC/27.1828.18284 \
+// RUN: /winsdkdir "%t/Windows Kits/10" \
+// RUN: -### -- %t/foo.cpp 2>&1 | FileCheck %s
 
-// CHECK: "-internal-isystem" "[[ROOT:[^"]*]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}include"
+// CHECK: "-internal-isystem" "[[ROOT:[^"]*]]{{/|}}DIA SDK{{/|}}include"
+// CHECK: "-internal-isystem" "[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}include"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}atlmfc{{/|}}include"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}ucrt"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}shared"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}um"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}winrt"
 
+// CHECK: "-libpath:[[ROOT]]{{/|}}DIA SDK{{/|}}lib{{/|}}amd64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}lib{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}atlmfc{{/|}}lib{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Lib{{/|}}10.0.19041.0{{/|}}ucrt{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Lib{{/|}}10.0.19041.0{{/|}}um{{/|}}x64"
+
 #--- VC/Tools/MSVC/27.1828.18284/include/string
 namespace std {
 class mystring {
@@ -24,6 +33,9 @@
 #--- Windows Kits/10/Include/10.0.19041.0/ucrt/assert.h
 #define myassert(X)
 
+#--- DIA SDK/include/cvconst.h
+#define myotherassert(X)
+
 #--- foo.cpp
 #include 
 #include 
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -63,6 +63,61 @@
 using namespace clang;
 using namespace llvm::opt;
 
+// Windows SDKs and VC Toolchains group their contents into subdirectories based
+// on the target architecture. This function converts an llvm::Triple::ArchType
+// to the corresponding subdirectory name.
+static const char *llvmArchToWindowsSDKArch(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
+  switch (Arch) {
+  case ArchType::x86:
+return "x86";
+  case ArchType::x86_64:
+return "x64";
+  case ArchType::arm:
+return "arm";
+  case ArchType::aarch64:
+return "arm64";
+  default:
+return "";
+  }
+}
+
+// Similar to the above function, but for Visual Studios before VS2017.
+static const char *llvmArchToLegacyVCArch(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
+  switch (Arch) {
+  case ArchType::x86:
+// x86 is default in legacy VC toolchains.
+// e.g. x86 libs are directly in /lib as opposed to /lib/x86.
+return "";
+  case ArchType::x86_64:
+return "amd64";
+  case ArchType::arm:
+return "arm";
+  case ArchType::aarch64:
+return "arm64";
+  default:
+return "";
+  }
+}
+
+// Similar to the above function, but for DevDiv internal builds.
+static const char *llvmArchToDevDivInternalArch(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
+  switch (Arch) {
+  case ArchType::x86:
+return "i386";
+  case ArchType::x86_64:
+return "amd64";
+  case ArchType::arm:
+return "arm";
+  case

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

2021-09-16 Thread Hans Wennborg via Phabricator via cfe-commits

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

In D109828#3002495 , @thakis wrote:

> In D109828#3002114 , @hans wrote:
>
>> The /winsysroot part makes sense to me, but what's the case for the new 
>> /diasdkdir flag?
>
>
>
> - If you do have a vcvars shell, you don't need the full sysroot path and 
> it's kind of useful (see example in commit message)
> - it seems nice to be able to explain /winsysroot as combination of other 
> flags in the help text
> - it makes writing the test a bit easier
> - it makes diasdkdir more like the other flags controlled by /winsysroot

Okay, sounds good to me.


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

https://reviews.llvm.org/D109828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

2021-09-15 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

In D109828#3002114 , @hans wrote:

> The /winsysroot part makes sense to me, but what's the case for the new 
> /diasdkdir flag?



- If you do have a vcvars shell, you don't need the full sysroot path and it's 
kind of useful (see example in commit message)
- it seems nice to be able to explain /winsysroot as combination of other flags 
in the help text
- it makes writing the test a bit easier
- it makes diasdkdir more like the other flags controlled by /winsysroot




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:66
 
+static const char *llvmArchToLegacyVCArch(llvm::Triple::ArchType Arch);
+

hans wrote:
> Maybe move the arch functions up to before visualstudio::Linker::ConstructJob 
> instead?
> 
> The file is already kind of organized in that way, with lots of static helper 
> functions on top, and then the implementations of the member functions.
Sure, done. The file has a bunch of other static functions all over the place 
too though :)


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

https://reviews.llvm.org/D109828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

2021-09-15 Thread Nico Weber via Phabricator via cfe-commits

thakis updated this revision to Diff 372782.
thakis marked an inline comment as done.
thakis added a comment.

move arch functions


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

https://reviews.llvm.org/D109828

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/test/Driver/cl-sysroot.cpp

Index: clang/test/Driver/cl-sysroot.cpp
===
--- clang/test/Driver/cl-sysroot.cpp
+++ clang/test/Driver/cl-sysroot.cpp
@@ -1,18 +1,27 @@
 // RUN: rm -rf %t
 // RUN: split-file %s %t
 
-// RUN: %clang_cl /winsysroot %t -### -- %t/foo.cpp 2>&1 | FileCheck %s
-// RUN: %clang_cl /vctoolsdir %t/VC/Tools/MSVC/27.1828.18284 \
-// RUN:   /winsdkdir "%t/Windows Kits/10" \
-// RUN:   -### -- %t/foo.cpp 2>&1 | FileCheck %s
+// RUN: %clang_cl -m64 /winsysroot %t -### -- %t/foo.cpp 2>&1 | FileCheck %s
+// RUN: %clang_cl -m64 \
+// RUN: /diasdkdir "%t/DIA SDK" \
+// RUN: /vctoolsdir %t/VC/Tools/MSVC/27.1828.18284 \
+// RUN: /winsdkdir "%t/Windows Kits/10" \
+// RUN: -### -- %t/foo.cpp 2>&1 | FileCheck %s
 
-// CHECK: "-internal-isystem" "[[ROOT:[^"]*]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}include"
+// CHECK: "-internal-isystem" "[[ROOT:[^"]*]]{{/|}}DIA SDK{{/|}}include"
+// CHECK: "-internal-isystem" "[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}include"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}atlmfc{{/|}}include"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}ucrt"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}shared"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}um"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}winrt"
 
+// CHECK: "-libpath:[[ROOT]]{{/|}}DIA SDK{{/|}}lib{{/|}}amd64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}lib{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}atlmfc{{/|}}lib{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Lib{{/|}}10.0.19041.0{{/|}}ucrt{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Lib{{/|}}10.0.19041.0{{/|}}um{{/|}}x64"
+
 #--- VC/Tools/MSVC/27.1828.18284/include/string
 namespace std {
 class mystring {
@@ -24,6 +33,9 @@
 #--- Windows Kits/10/Include/10.0.19041.0/ucrt/assert.h
 #define myassert(X)
 
+#--- DIA SDK/include/cvconst.h
+#define myotherassert(X)
+
 #--- foo.cpp
 #include 
 #include 
@@ -31,4 +43,5 @@
 void f() {
   std::mystring s;
   myassert(s.empty());
+  myotherassert(s.empty());
 }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -63,6 +63,62 @@
 using namespace clang;
 using namespace llvm::opt;
 
+// Windows SDKs and VC Toolchains group their contents into subdirectories based
+// on the target architecture. This function converts an llvm::Triple::ArchType
+// to the corresponding subdirectory name.
+static const char *llvmArchToWindowsSDKArch(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
+  switch (Arch) {
+  case ArchType::x86:
+return "x86";
+  case ArchType::x86_64:
+return "x64";
+  case ArchType::arm:
+return "arm";
+  case ArchType::aarch64:
+return "arm64";
+  default:
+return "";
+  }
+}
+
+// Similar to the above function, but for Visual Studios before VS2017.
+static const char *llvmArchToLegacyVCArch(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
+  switch (Arch) {
+  case ArchType::x86:
+// x86 is default in legacy VC toolchains.
+// e.g. x86 libs are directly in /lib as opposed to /lib/x86.
+return "";
+  case ArchType::x86_64:
+return "amd64";
+  case ArchType::arm:
+return "arm";
+  case ArchType::aarch64:
+return "arm64";
+  default:
+return "";
+  }
+}
+
+// Similar to the above function, but for DevDiv internal builds.
+static const char *llvmArchToDevDivInternalArch(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
+  switch (Arch) {
+  case ArchType::x86:
+return "i386";
+  case ArchType::x86_64:
+return "amd64";
+  case ArchType::arm:
+return "arm";
+  case ArchType::aarch64:
+return "arm64";
+  default:
+return "";
+  }
+}
+
+
 static bool canExecute(llvm::vfs::FileSystem , StringRef Path) {
   auto Status =

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

2021-09-15 Thread Hans Wennborg via Phabricator via cfe-commits

hans added a comment.

The /winsysroot part makes sense to me, but what's the case for the new 
/diasdkdir flag?




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:66
 
+static const char *llvmArchToLegacyVCArch(llvm::Triple::ArchType Arch);
+

Maybe move the arch functions up to before visualstudio::Linker::ConstructJob 
instead?

The file is already kind of organized in that way, with lots of static helper 
functions on top, and then the implementations of the member functions.


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

https://reviews.llvm.org/D109828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

2021-09-15 Thread Nico Weber via Phabricator via cfe-commits

thakis created this revision.
thakis added a reviewer: hans.
Herald added subscribers: dang, pengfei.
thakis requested review of this revision.

D109708  added "DIA SDK" to our win sysroot 
for hermetic builds
that use LLVM_ENABLE_DIA_SDK. But the build system still has to
manually pass flags pointing to it.

Since we have a /winsysroot flag, make it look at DIA SDK in
the sysroot.

With this, the following is enough to compile the DIA2Dump example:

out\gn\bin\clang-cl ^

  "sysroot\DIA SDK\Samples\DIA2Dump\DIA2Dump.cpp" ^
  "sysroot\DIA SDK\Samples\DIA2Dump\PrintSymbol.cpp" ^
  "sysroot\DIA SDK\Samples\DIA2Dump\regs.cpp" ^
  /diasdkdir "sysroot\DIA SDK" ^
  ole32.lib oleaut32.lib diaguids.lib

---

Given that the DIA SDK is part of the MSVC installation (at ` C:\Program Files 
(x86)\Microsoft Visual Studio\2017\Professional\DIA SDK` on my machine), I 
think there's a case for letting clang-cl know about it. Arguably, adding `/I 
sysroot\DIA SDK\include` to your cflags isn't super difficult, but one day 
we'll teach lld-link about /winsysroot too, and then it means you don't need to 
know about the `lib\` folder layout inside DIA SDK. Also, if you link through 
clang-cl instead of calling the linker directly, you get that simplification 
with this change already. That's nice for local one-off commands, but in 
practice most projects admittedly call link.exe directly.

So all-in-all, I think this is a good idea, but I could be convinced otherwise 
if others disagree.


https://reviews.llvm.org/D109828

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/test/Driver/cl-sysroot.cpp

Index: clang/test/Driver/cl-sysroot.cpp
===
--- clang/test/Driver/cl-sysroot.cpp
+++ clang/test/Driver/cl-sysroot.cpp
@@ -1,18 +1,27 @@
 // RUN: rm -rf %t
 // RUN: split-file %s %t
 
-// RUN: %clang_cl /winsysroot %t -### -- %t/foo.cpp 2>&1 | FileCheck %s
-// RUN: %clang_cl /vctoolsdir %t/VC/Tools/MSVC/27.1828.18284 \
-// RUN:   /winsdkdir "%t/Windows Kits/10" \
-// RUN:   -### -- %t/foo.cpp 2>&1 | FileCheck %s
+// RUN: %clang_cl -m64 /winsysroot %t -### -- %t/foo.cpp 2>&1 | FileCheck %s
+// RUN: %clang_cl -m64 \
+// RUN: /diasdkdir "%t/DIA SDK" \
+// RUN: /vctoolsdir %t/VC/Tools/MSVC/27.1828.18284 \
+// RUN: /winsdkdir "%t/Windows Kits/10" \
+// RUN: -### -- %t/foo.cpp 2>&1 | FileCheck %s
 
-// CHECK: "-internal-isystem" "[[ROOT:[^"]*]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}include"
+// CHECK: "-internal-isystem" "[[ROOT:[^"]*]]{{/|}}DIA SDK{{/|}}include"
+// CHECK: "-internal-isystem" "[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}include"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}atlmfc{{/|}}include"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}ucrt"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}shared"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}um"
 // CHECK: "-internal-isystem" "[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Include{{/|}}10.0.19041.0{{/|}}winrt"
 
+// CHECK: "-libpath:[[ROOT]]{{/|}}DIA SDK{{/|}}lib{{/|}}amd64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}lib{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}VC{{/|}}Tools{{/|}}MSVC{{/|}}27.1828.18284{{/|}}atlmfc{{/|}}lib{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Lib{{/|}}10.0.19041.0{{/|}}ucrt{{/|}}x64"
+// CHECK: "-libpath:[[ROOT]]{{/|}}Windows Kits{{/|}}10{{/|}}Lib{{/|}}10.0.19041.0{{/|}}um{{/|}}x64"
+
 #--- VC/Tools/MSVC/27.1828.18284/include/string
 namespace std {
 class mystring {
@@ -24,6 +33,9 @@
 #--- Windows Kits/10/Include/10.0.19041.0/ucrt/assert.h
 #define myassert(X)
 
+#--- DIA SDK/include/cvconst.h
+#define myotherassert(X)
+
 #--- foo.cpp
 #include 
 #include 
@@ -31,4 +43,5 @@
 void f() {
   std::mystring s;
   myassert(s.empty());
+  myotherassert(s.empty());
 }
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -63,6 +63,8 @@
 using namespace clang;
 using namespace llvm::opt;
 
+static const char *llvmArchToLegacyVCArch(llvm::Triple::ArchType Arch);
+
 static bool canExecute(llvm::vfs::FileSystem , StringRef Path) {
   auto Status = VFS.status(Path);
   if (!Status)
@@ -396,6 +398,20 @@
   // the environment variable is set however, assume the user knows what

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

[PATCH] D109828: [clang-cl] Add a /diasdkdir flag and make /winsysroot imply it

6 matches

Site Navigation

Mail list logo

Footer information