[PATCH] D157331: [clang] Implement C23

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This change broke building a recent version of gettext. Gettext uses gnulib for 
polyfilling various utility functions. Since not long ago, these functions 
internally use ``, 
https://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/malloca.c?id=ef5a4088d9236a55283d1eb576f560aa39c09e6f.
 If the toolchain doesn't provide the header, gnulib provides a fallback 
version of its own: 
https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/stdckdint.in.h

Now since Clang provides it since this commit, gnulib doesn't provide their own 
fallback, but goes on to use `ckd_add`. This fails with Clang's `` 
implementation, since gnulib isn't built in C23 mode but still wants to use 
``:

  i686-w64-mingw32-clang -DHAVE_CONFIG_H -DEXEEXT=\".exe\" -I. 
-I../../../gettext-runtime/gnulib-lib -I..  -I../intl 
-I../../../gettext-runtime/intl -DDEPENDS_ON_LIBICONV=1 -DDEPENDS_ON_LIBINTL=1 
-I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include  -Wno-cast-qual 
-Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef 
-Wno-unused-function -Wno-unused-parameter -Wno-float-conversion 
-Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits 
-I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include -g -O2 -c -o 
libgrt_a-malloca.o `test -f 'malloca.c' || echo 
'../../../gettext-runtime/gnulib-lib/'`malloca.c
  ../../../gettext-runtime/gnulib-lib/malloca.c:53:8: error: call to undeclared 
function 'ckd_add'; ISO C99 and later do not support implicit function 
declarations [-Wimplicit-function-declaration]
 53 |   if (!ckd_add (, n, plus) && !xalloc_oversized (nplus, 1))
|^
  1 error generated.
  make: *** [Makefile:2246: libgrt_a-malloca.o] Error 1

It seems like GCC's version of this header exposes the functionality even if 
compiled in an older language mode: 
https://github.com/gcc-mirror/gcc/blob/f019251ac9be017aaf3c58f87f43d88b974d21cf/gcc/ginclude/stdckdint.h

Would you consider changing the Clang version to do the same? Otherwise we 
would need to convince gnulib to not try to use/polyfill this header unless 
gnulib itself is compiled in C23 mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: openmp/runtime/test/target/target_thread_limit.cpp:28
+// OMP51: target: parallel
+// OMP51: target: parallel
+// OMP51-NOT: target: parallel

sandeepkosuri wrote:
> mstorsjo wrote:
> > mstorsjo wrote:
> > > This test fails when running (on Windows) on GitHub Actions runners - see 
> > > https://github.com/mstorsjo/llvm-mingw/actions/runs/6019088705/job/16342540379.
> > > 
> > > I believe that this bit of the test has got a hidden assumption that it 
> > > is running in an environment with 4 or more cores. By setting `#pragma 
> > > omp target thread_limit(tl)` (with `tl=4`) and running a line in parallel 
> > > with `#pragma omp parallel`, it expects that we'll get 4 printouts - 
> > > while in practice, we'll get anywhere between 1 and 4 printouts depending 
> > > on the number of cores.
> > > 
> > > Is there something that can be done to make this test work in such an 
> > > environment too?
> > Can someone involved in this patch take on fixing it so that it works on 
> > machines with fewer than 4 cores? I'm not sure what's the most appropriate 
> > path forward here, as it breaks clearly in such configs (even if it might 
> > not be hit by one of the official llvm buildbots, but it shows up as 
> > breakage in my nightly builds every day now) - reverting seems a bit harsh. 
> > I guess I could just rip out this part of the test?
> @mstorsjo , I noticed that you have committed this 
> https://github.com/llvm/llvm-project/commit/c2019c416c8d7ec50aec6ac6b82c9aa4e99b0f6f
> 
> Does this solve your problem ?
Yes, that commit fixed the issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

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

[PATCH] D61670: [clang] [MinGW] Add the option -fno-auto-import

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf9f2fdcf03d9: [clang] [MinGW] Add the option 
-fno-auto-import (authored by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D61670?vs=553624=555462#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/test/CodeGen/dso-local-executable.c
  clang/test/Driver/mingw-auto-import.c

Index: clang/test/Driver/mingw-auto-import.c
===
--- /dev/null
+++ clang/test/Driver/mingw-auto-import.c
@@ -0,0 +1,15 @@
+// By default, we don't pass any -fauto-import to -cc1, as that's the default.
+//
+// RUN: %clang --target=x86_64-w64-windows-gnu -### %s 2>&1 | FileCheck --check-prefixes=DEFAULT %s
+// RUN: %clang --target=x86_64-w64-windows-gnu -fno-auto-import -fauto-import -### %s 2>&1 | FileCheck --check-prefixes=DEFAULT %s
+// DEFAULT: "-cc1"
+// DEFAULT-NOT: no-auto-import
+// DEFAULT-NOT: --disable-auto-import
+
+// When compiling with -fno-auto-import, we pass -fno-auto-import to -cc1
+// and --disable-auto-import to the linker.
+//
+// RUN: %clang --target=x86_64-w64-windows-gnu -fauto-import -fno-auto-import -### %s 2>&1 | FileCheck --check-prefixes=NO_AUTOIMPORT %s
+// NO_AUTOIMPORT: "-cc1"
+// NO_AUTOIMPORT: "-fno-auto-import"
+// NO_AUTOIMPORT: "--disable-auto-import"
Index: clang/test/CodeGen/dso-local-executable.c
===
--- clang/test/CodeGen/dso-local-executable.c
+++ clang/test/CodeGen/dso-local-executable.c
@@ -9,12 +9,14 @@
 // COFF-DAG: define dso_local ptr @zed()
 // COFF-DAG: declare dllimport void @import_func()
 
-// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS %s
-// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -femulated-tls | FileCheck --check-prefixes=MINGW,MINGW-EMUTLS %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS,MINGW-AUTO-IMPORT %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -fno-auto-import | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS,MINGW-NO-AUTO-IMPORT %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -femulated-tls | FileCheck --check-prefixes=MINGW,MINGW-EMUTLS,MINGW-AUTO-IMPORT %s
 // MINGW:  @baz = dso_local global i32 42
 // MINGW-NEXT: @import_var = external dllimport global i32
 // MINGW-NEXT: @weak_bar = extern_weak global i32
-// MINGW-NEXT: @bar = external global i32
+// MINGW-AUTO-IMPORT-NEXT: @bar = external global i32
+// MINGW-NO-AUTO-IMPORT-NEXT: @bar = external dso_local global i32
 // MINGW-NEXT: @local_thread_var = dso_local thread_local global i32 42
 // MINGW-NATIVE_TLS-NEXT: @thread_var = external dso_local thread_local global i32
 // MINGW-EMUTLS-NEXT: @thread_var = external thread_local global i32
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -169,6 +169,10 @@
   if (Args.hasArg(options::OPT_Z_Xlinker__no_demangle))
 CmdArgs.push_back("--no-demangle");
 
+  if (!Args.hasFlag(options::OPT_fauto_import, options::OPT_fno_auto_import,
+true))
+CmdArgs.push_back("--disable-auto-import");
+
   if (Arg *A = Args.getLastArg(options::OPT_mguard_EQ)) {
 StringRef GuardArgs = A->getValue();
 if (GuardArgs == "none")
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5623,6 +5623,11 @@
 CmdArgs.push_back("-mms-bitfields");
   }
 
+  if (Triple.isWindowsGNUEnvironment()) {
+Args.addOptOutFlag(CmdArgs, options::OPT_fauto_import,
+   options::OPT_fno_auto_import);
+  }
+
   // Non-PIC code defaults to -fdirect-access-external-data while PIC code
   // defaults to -fno-direct-access-external-data. Pass the option if different
   // from the default.
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1419,6 +1419,7 @@
 return false;
 
   const llvm::Triple  = CGM.getTriple();
+  const auto  = CGM.getCodeGenOpts();
   if (TT.isWindowsGNUEnvironment()) {
 // In MinGW, variables without DLLImport can still be automatically
 // imported from a DLL by the linker; don't mark variables that
@@ -1429,7 

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: openmp/runtime/test/target/target_thread_limit.cpp:28
+// OMP51: target: parallel
+// OMP51: target: parallel
+// OMP51-NOT: target: parallel

mstorsjo wrote:
> This test fails when running (on Windows) on GitHub Actions runners - see 
> https://github.com/mstorsjo/llvm-mingw/actions/runs/6019088705/job/16342540379.
> 
> I believe that this bit of the test has got a hidden assumption that it is 
> running in an environment with 4 or more cores. By setting `#pragma omp 
> target thread_limit(tl)` (with `tl=4`) and running a line in parallel with 
> `#pragma omp parallel`, it expects that we'll get 4 printouts - while in 
> practice, we'll get anywhere between 1 and 4 printouts depending on the 
> number of cores.
> 
> Is there something that can be done to make this test work in such an 
> environment too?
Can someone involved in this patch take on fixing it so that it works on 
machines with fewer than 4 cores? I'm not sure what's the most appropriate path 
forward here, as it breaks clearly in such configs (even if it might not be hit 
by one of the official llvm buildbots, but it shows up as breakage in my 
nightly builds every day now) - reverting seems a bit harsh. I guess I could 
just rip out this part of the test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

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

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: openmp/runtime/test/target/target_thread_limit.cpp:28
+// OMP51: target: parallel
+// OMP51: target: parallel
+// OMP51-NOT: target: parallel

This test fails when running (on Windows) on GitHub Actions runners - see 
https://github.com/mstorsjo/llvm-mingw/actions/runs/6019088705/job/16342540379.

I believe that this bit of the test has got a hidden assumption that it is 
running in an environment with 4 or more cores. By setting `#pragma omp target 
thread_limit(tl)` (with `tl=4`) and running a line in parallel with `#pragma 
omp parallel`, it expects that we'll get 4 printouts - while in practice, we'll 
get anywhere between 1 and 4 printouts depending on the number of cores.

Is there something that can be done to make this test work in such an 
environment too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

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

[PATCH] D152752: [MS] Fix passing aligned records by value in some cases

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D152752#4626234 , @rnk wrote:

> I need to get to it, my recollection is that @mstorsjo ran into the same 
> issue here for mingw and made some changes, I wanted to go dig those up as a 
> starting point. I may have completely forgotten things though.

Hmm, I don't remember doing anything in that area - I don't think I've had to 
touch variadics on i386 (or x86_64 for that matter) so far, or anything 
relating to aligned variadics. (The main thing that might sound similar to 
alignment was about setting up the homed registers on aarch64 when receiving 
variadics; there's some special casing there relating to whether the number of 
homed registers is even or odd. But all of that is much deeper within lowering 
in LLVM.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152752

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

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D61670#4621724 , @aeubanks wrote:

> I don't have all the context here, but seems fine once the commit description 
> is updated with the new spelling

Thanks. Yeah I've updated the commit message locally (I wonder if the `arc` 
tool can resync the patch description? Although that's becoming irrelevant real 
soon now anyway.) I'll update the description here anyway, and I'll probably go 
ahead and push it in a few days if nobody else wants to comment on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

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

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D152054#4622642 , @vadikp-intel 
wrote:

> Windows importing is now done by name, and new exports do not need to have an 
> ordinal specified for them i.e. you can add a line with just the API name to 
> dllexports.

Oh, right, thanks. Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

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

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added subscribers: vadikp-intel, natgla.
mstorsjo added a comment.

In D152054#4620927 , @mstorsjo wrote:

> This new test is failing on Windows, due to `__kmpc_set_thread_limit` not 
> being exported - see e.g. 
> https://github.com/mstorsjo/llvm-mingw/actions/runs/5994183421/job/16264501555.
>  Can someone add it to `openmp/runtime/src/dllexports`?

CC @vadikp-intel @natgla about this. What's the procedure for allocating new 
ordinal numbers for new exported functions here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

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

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This new test is failing on Windows, due to `__kmpc_set_thread_limit` not being 
exported - see e.g. 
https://github.com/mstorsjo/llvm-mingw/actions/runs/5994183421/job/16264501555. 
Can someone add it to `openmp/runtime/src/dllexports`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

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

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 553624.
mstorsjo edited the summary of this revision.
mstorsjo added a comment.

Updated to use the form -fno-auto-import and similar split it into two separate 
words everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/test/CodeGen/dso-local-executable.c
  clang/test/Driver/mingw-auto-import.c

Index: clang/test/Driver/mingw-auto-import.c
===
--- /dev/null
+++ clang/test/Driver/mingw-auto-import.c
@@ -0,0 +1,15 @@
+// By default, we don't pass any -fauto-import to -cc1, as that's the default.
+//
+// RUN: %clang --target=x86_64-w64-windows-gnu -### %s 2>&1 | FileCheck --check-prefixes=DEFAULT %s
+// RUN: %clang --target=x86_64-w64-windows-gnu -fno-auto-import -fauto-import -### %s 2>&1 | FileCheck --check-prefixes=DEFAULT %s
+// DEFAULT: "-cc1"
+// DEFAULT-NOT: no-auto-import
+// DEFAULT-NOT: --disable-auto-import
+
+// When compiling with -fno-auto-import, we pass -fno-auto-import to -cc1
+// and --disable-auto-import to the linker.
+//
+// RUN: %clang --target=x86_64-w64-windows-gnu -fauto-import -fno-auto-import -### %s 2>&1 | FileCheck --check-prefixes=NO_AUTOIMPORT %s
+// NO_AUTOIMPORT: "-cc1"
+// NO_AUTOIMPORT: "-fno-auto-import"
+// NO_AUTOIMPORT: "--disable-auto-import"
Index: clang/test/CodeGen/dso-local-executable.c
===
--- clang/test/CodeGen/dso-local-executable.c
+++ clang/test/CodeGen/dso-local-executable.c
@@ -9,12 +9,14 @@
 // COFF-DAG: define dso_local ptr @zed()
 // COFF-DAG: declare dllimport void @import_func()
 
-// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS %s
-// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -femulated-tls | FileCheck --check-prefixes=MINGW,MINGW-EMUTLS %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS,MINGW-AUTO-IMPORT %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -fno-auto-import | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS,MINGW-NO-AUTO-IMPORT %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -femulated-tls | FileCheck --check-prefixes=MINGW,MINGW-EMUTLS,MINGW-AUTO-IMPORT %s
 // MINGW:  @baz = dso_local global i32 42
 // MINGW-NEXT: @import_var = external dllimport global i32
 // MINGW-NEXT: @weak_bar = extern_weak global i32
-// MINGW-NEXT: @bar = external global i32
+// MINGW-AUTO-IMPORT-NEXT: @bar = external global i32
+// MINGW-NO-AUTO-IMPORT-NEXT: @bar = external dso_local global i32
 // MINGW-NEXT: @local_thread_var = dso_local thread_local global i32 42
 // MINGW-NATIVE_TLS-NEXT: @thread_var = external dso_local thread_local global i32
 // MINGW-EMUTLS-NEXT: @thread_var = external thread_local global i32
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -169,6 +169,10 @@
   if (Args.hasArg(options::OPT_Z_Xlinker__no_demangle))
 CmdArgs.push_back("--no-demangle");
 
+  if (!Args.hasFlag(options::OPT_fauto_import, options::OPT_fno_auto_import,
+true))
+CmdArgs.push_back("--disable-auto-import");
+
   if (Arg *A = Args.getLastArg(options::OPT_mguard_EQ)) {
 StringRef GuardArgs = A->getValue();
 if (GuardArgs == "none")
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5622,6 +5622,11 @@
 CmdArgs.push_back("-mms-bitfields");
   }
 
+  if (Triple.isWindowsGNUEnvironment()) {
+Args.addOptOutFlag(CmdArgs, options::OPT_fauto_import,
+   options::OPT_fno_auto_import);
+  }
+
   // Non-PIC code defaults to -fdirect-access-external-data while PIC code
   // defaults to -fno-direct-access-external-data. Pass the option if different
   // from the default.
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1409,6 +1409,7 @@
 return false;
 
   const llvm::Triple  = CGM.getTriple();
+  const auto  = CGM.getCodeGenOpts();
   if (TT.isWindowsGNUEnvironment()) {
 // In MinGW, variables without DLLImport can still be automatically
 // imported from a DLL by the linker; don't mark variables that
@@ -1419,7 +1420,8 @@
 // such variables can't be 

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D61670#4611790 , @aeubanks wrote:

> In D61670#4607313 , @mstorsjo wrote:
>
>>> I expected the answer would be "yes", so I said "lgtm" and then phrased my 
>>> question very awkwardly.
>>
>> Ah, thanks for the clarification!
>>
>> Any opinion on the name, `-fno-autoimport` vs `-fno-auto-import`, given the 
>> existing linker option `--disable-auto-import`?
>
> Is there an official name, "auto-import"/"autoimport"/"auto import"?

Not really, no - it's all very much ad-hoc.

> Since cc1 flags are changeable and this hasn't landed yet, I think it'd be 
> nice to standardize before we commit on a stable name.
> If there isn't already a name, I like the dash in "auto-import".

Ok, sounds reasonable - I'll change it into that form then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

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

[PATCH] D158411: [clang] [MinGW] Pass LTO options to the linker

[Martin Storsjö 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 rGa23bf1786be7: [clang] [MinGW] Pass LTO options to the linker 
(authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158411

Files:
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/test/Driver/mingw-lto.c


Index: clang/test/Driver/mingw-lto.c
===
--- clang/test/Driver/mingw-lto.c
+++ clang/test/Driver/mingw-lto.c
@@ -1,4 +1,6 @@
 // The default linker doesn't support LLVM bitcode
 // RUN: not %clang --target=i686-pc-windows-gnu %s -flto -fuse-ld=bfd
 // When using lld, this is allowed though.
-// RUN: %clang --target=i686-pc-windows-gnu -### %s -flto -fuse-ld=lld 
-B%S/Inputs/lld
+// RUN: %clang --target=i686-pc-windows-gnu -### %s -flto -fuse-ld=lld 
-B%S/Inputs/lld -femulated-tls 2>&1 | FileCheck %s
+
+// CHECK: "-plugin-opt=-emulated-tls"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -238,6 +238,12 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (D.isUsingLTO()) {
+assert(!Inputs.empty() && "Must have at least one input.");
+addLTOOptions(TC, Args, CmdArgs, Output, Inputs[0],
+  D.getLTOMode() == LTOK_Thin);
+  }
+
   if (C.getDriver().IsFlangMode()) {
 addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
 addFortranRuntimeLibs(TC, CmdArgs);


Index: clang/test/Driver/mingw-lto.c
===
--- clang/test/Driver/mingw-lto.c
+++ clang/test/Driver/mingw-lto.c
@@ -1,4 +1,6 @@
 // The default linker doesn't support LLVM bitcode
 // RUN: not %clang --target=i686-pc-windows-gnu %s -flto -fuse-ld=bfd
 // When using lld, this is allowed though.
-// RUN: %clang --target=i686-pc-windows-gnu -### %s -flto -fuse-ld=lld -B%S/Inputs/lld
+// RUN: %clang --target=i686-pc-windows-gnu -### %s -flto -fuse-ld=lld -B%S/Inputs/lld -femulated-tls 2>&1 | FileCheck %s
+
+// CHECK: "-plugin-opt=-emulated-tls"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -238,6 +238,12 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (D.isUsingLTO()) {
+assert(!Inputs.empty() && "Must have at least one input.");
+addLTOOptions(TC, Args, CmdArgs, Output, Inputs[0],
+  D.getLTOMode() == LTOK_Thin);
+  }
+
   if (C.getDriver().IsFlangMode()) {
 addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
 addFortranRuntimeLibs(TC, CmdArgs);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

> I expected the answer would be "yes", so I said "lgtm" and then phrased my 
> question very awkwardly.

Ah, thanks for the clarification!

Any opinion on the name, `-fno-autoimport` vs `-fno-auto-import`, given the 
existing linker option `--disable-auto-import`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

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

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D61670#4604145 , @rnk wrote:

> cc +@aeubanks @jyknight to consider using the code model for this purpose

Hmm, I don't quite understand this comment; do you suggest that we after all 
should use the code model for controlling the use of `.refptr` stubs too - 
didn't we conclude earlier that it isn't a great fit for that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

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

[PATCH] D158411: [clang] [MinGW] Pass LTO options to the linker

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added a reviewer: MaskRay.
Herald added subscribers: ormris, steven_wu, hiraditya, inglorion.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: clang.

This matches what is done on other platforms too.

This fixes one part of
https://github.com/mstorsjo/llvm-mingw/issues/349.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158411

Files:
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/test/Driver/mingw-lto.c


Index: clang/test/Driver/mingw-lto.c
===
--- clang/test/Driver/mingw-lto.c
+++ clang/test/Driver/mingw-lto.c
@@ -1,4 +1,6 @@
 // The default linker doesn't support LLVM bitcode
 // RUN: not %clang --target=i686-pc-windows-gnu %s -flto -fuse-ld=bfd
 // When using lld, this is allowed though.
-// RUN: %clang --target=i686-pc-windows-gnu -### %s -flto -fuse-ld=lld 
-B%S/Inputs/lld
+// RUN: %clang --target=i686-pc-windows-gnu -### %s -flto -fuse-ld=lld 
-B%S/Inputs/lld -femulated-tls 2>&1 | FileCheck %s
+
+// CHECK: "-plugin-opt=-emulated-tls"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -238,6 +238,12 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (D.isUsingLTO()) {
+assert(!Inputs.empty() && "Must have at least one input.");
+addLTOOptions(TC, Args, CmdArgs, Output, Inputs[0],
+  D.getLTOMode() == LTOK_Thin);
+  }
+
   if (C.getDriver().IsFlangMode()) {
 addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
 addFortranRuntimeLibs(TC, CmdArgs);


Index: clang/test/Driver/mingw-lto.c
===
--- clang/test/Driver/mingw-lto.c
+++ clang/test/Driver/mingw-lto.c
@@ -1,4 +1,6 @@
 // The default linker doesn't support LLVM bitcode
 // RUN: not %clang --target=i686-pc-windows-gnu %s -flto -fuse-ld=bfd
 // When using lld, this is allowed though.
-// RUN: %clang --target=i686-pc-windows-gnu -### %s -flto -fuse-ld=lld -B%S/Inputs/lld
+// RUN: %clang --target=i686-pc-windows-gnu -### %s -flto -fuse-ld=lld -B%S/Inputs/lld -femulated-tls 2>&1 | FileCheck %s
+
+// CHECK: "-plugin-opt=-emulated-tls"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -238,6 +238,12 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (D.isUsingLTO()) {
+assert(!Inputs.empty() && "Must have at least one input.");
+addLTOOptions(TC, Args, CmdArgs, Output, Inputs[0],
+  D.getLTOMode() == LTOK_Thin);
+  }
+
   if (C.getDriver().IsFlangMode()) {
 addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
 addFortranRuntimeLibs(TC, CmdArgs);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1477
+  "automatic dllimport, and enable support for it in the linker. "
+  "Enabled by default.">>;
+} // let Flags = [TargetSpecific]

I see that most similar `BoolFOption`s for features that are `DefaultTrue` only 
document the `NegFlag`, which makes only the negative flag show up in `clang 
--help`. This is probably what one most commonly wants - but I see that the 
option ends up undocumented in `ClangCommandLineReference.rst` unless it has 
documentation for the `PosFlag`. So therefore I wrote documentation for both 
options here, and in a form where the `PosFlag` docs mostly is intended for the 
generated reference docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

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

[PATCH] D61670: [RFC] [MinGW] Allow opting out from .refptr stubs

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 551957.
mstorsjo added a comment.
Herald added a subscriber: MaskRay.

Updated as discussed, adding a new option `-fno-autoimport` (and the 
corresponding positive one, `-fautoimport`, which is the implicit default), 
which affects code generation and linking.

The existing linker option `--disable-auto-import` (which is passed if this new 
option is set), splits "auto import" into two words - should the new Clang 
option try to harmonize with this spelling, making it `-fno-auto-import` 
instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/test/CodeGen/dso-local-executable.c
  clang/test/Driver/mingw-autoimport.c

Index: clang/test/Driver/mingw-autoimport.c
===
--- /dev/null
+++ clang/test/Driver/mingw-autoimport.c
@@ -0,0 +1,15 @@
+// By default, we don't pass any -fautoimport to -cc1, as that's the default.
+//
+// RUN: %clang --target=x86_64-w64-windows-gnu -### %s 2>&1 | FileCheck --check-prefixes=DEFAULT %s
+// RUN: %clang --target=x86_64-w64-windows-gnu -fno-autoimport -fautoimport -### %s 2>&1 | FileCheck --check-prefixes=DEFAULT %s
+// DEFAULT: "-cc1"
+// DEFAULT-NOT: no-autoimport
+// DEFAULT-NOT: --disable-auto-import
+
+// When compiling with -fno-autoimport, we pass -fno-autoimport to -cc1
+// and --disable-auto-import to the linker.
+//
+// RUN: %clang --target=x86_64-w64-windows-gnu -fautoimport -fno-autoimport -### %s 2>&1 | FileCheck --check-prefixes=NO_AUTOIMPORT %s
+// NO_AUTOIMPORT: "-cc1"
+// NO_AUTOIMPORT: "-fno-autoimport"
+// NO_AUTOIMPORT: "--disable-auto-import"
Index: clang/test/CodeGen/dso-local-executable.c
===
--- clang/test/CodeGen/dso-local-executable.c
+++ clang/test/CodeGen/dso-local-executable.c
@@ -9,12 +9,14 @@
 // COFF-DAG: define dso_local ptr @zed()
 // COFF-DAG: declare dllimport void @import_func()
 
-// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS %s
-// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -femulated-tls | FileCheck --check-prefixes=MINGW,MINGW-EMUTLS %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS,MINGW-AUTOIMPORT %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -fno-autoimport | FileCheck --check-prefixes=MINGW,MINGW-NATIVE_TLS,MINGW-NO-AUTOIMPORT %s
+// RUN: %clang_cc1 -triple x86_64-w64-mingw32 -emit-llvm %s -o - -femulated-tls | FileCheck --check-prefixes=MINGW,MINGW-EMUTLS,MINGW-AUTOIMPORT %s
 // MINGW:  @baz = dso_local global i32 42
 // MINGW-NEXT: @import_var = external dllimport global i32
 // MINGW-NEXT: @weak_bar = extern_weak global i32
-// MINGW-NEXT: @bar = external global i32
+// MINGW-AUTOIMPORT-NEXT: @bar = external global i32
+// MINGW-NO-AUTOIMPORT-NEXT: @bar = external dso_local global i32
 // MINGW-NEXT: @local_thread_var = dso_local thread_local global i32 42
 // MINGW-NATIVE_TLS-NEXT: @thread_var = external dso_local thread_local global i32
 // MINGW-EMUTLS-NEXT: @thread_var = external thread_local global i32
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -169,6 +169,10 @@
   if (Args.hasArg(options::OPT_Z_Xlinker__no_demangle))
 CmdArgs.push_back("--no-demangle");
 
+  if (!Args.hasFlag(options::OPT_fautoimport, options::OPT_fno_autoimport,
+true))
+CmdArgs.push_back("--disable-auto-import");
+
   if (Arg *A = Args.getLastArg(options::OPT_mguard_EQ)) {
 StringRef GuardArgs = A->getValue();
 if (GuardArgs == "none")
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5623,6 +5623,11 @@
 CmdArgs.push_back("-mms-bitfields");
   }
 
+  if (Triple.isWindowsGNUEnvironment()) {
+Args.addOptOutFlag(CmdArgs, options::OPT_fautoimport,
+   options::OPT_fno_autoimport);
+  }
+
   // Non-PIC code defaults to -fdirect-access-external-data while PIC code
   // defaults to -fno-direct-access-external-data. Pass the option if different
   // from the default.
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1409,6 +1409,7 @@
 return false;
 
   const 

[PATCH] D157762: [WIP] Implement [[msvc::no_unique_address]]

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1409
+
+  On Windows targets, ``[[no_unique_address]]`` is ignored; use
+  ``[[msvc::no_unique_address]]`` instead.

On MSVC targets, `[[no_unique_address]]` is ignored - it's not ignored for 
mingw targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157762

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö 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 rGd60c3d08e78d: [clang] Skip stores in init for fields that 
are empty structs (authored by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D157332?vs=549575=550209#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | 
FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated 
for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns 
as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5781,9 +5782,14 @@
 DestIsVolatile = false;
   }
 
-  // If the value is offset in memory, apply the offset now.
-  Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-  CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  // An empty record can overlap other data (if declared with
+  // no_unique_address); omit the store for such types - as there is no
+  // actual data to store.
+  if (!isEmptyRecord(getContext(), RetTy, true)) {
+// If the value is offset in memory, apply the offset now.
+Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
+CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  }
 
   return convertTempToRValue(DestPtr, RetTy, SourceLocation());
 }


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 

[PATCH] D157794: [Driver] Make /Zi and /Z7 aliases of -g rather than handling them specially

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/test/Driver/cl-options.c:567
 
-// This test was super sneaky: "/Z7" means "line-tables", but "-gdwarf" occurs
-// later on the command line, so it should win. Interestingly the cc1 arguments
-// came out right, but had wrong semantics, because an invariant assumed by
-// CompilerInvocation was violated: it expects that at most one of {gdwarfN,
-// line-tables-only} appear. If you assume that, then you can safely use
-// Args.hasArg to test whether a boolean flag is present without caring
-// where it appeared. And for this test, it appeared to the left of -gdwarf
-// which made it "win". This test could not detect that bug.
+// If We specify both /Z7 and -gdwarf we should get dwarf, not codeview.
 // RUN: %clang_cl /Z7 -gdwarf /c -### -- %s 2>&1 | FileCheck 
-check-prefix=Z7_gdwarf %s

smeenai wrote:
> bogner wrote:
> > smeenai wrote:
> > > bogner wrote:
> > > > rnk wrote:
> > > > > I think Meta folks depend on a mode that emits both codeview and 
> > > > > DWARF. +@smeenai
> > > > Hopefully if that's the case there's a better test somewhere than this 
> > > > one that discusses in detail how `/Z7` is an alias for 
> > > > `-gline-tables-only`, which hasn't been true since 2017.
> > > Thanks for the headers up :) We don't depend on that mode any more 
> > > though, so no problems on our end.
> > Also note that this change does not affect what happens if we specify both 
> > `-gcodeview` and `-gdwarf` together. That continues to pass both 
> > `-gcodeview` and `-dwarf-version=...` to `clang -cc1`. However, I don't see 
> > any tests that actually check that behaviour, so if that is a mode that's 
> > useful to keep working we definitely have some gaps there.
> > 
> > This change means that if you want to ask `-cc1` for dwarf *and* codeview 
> > with `/Z7` or `/Zi`, you'll need to specify `-gdwarf` and `-gcodeview` now. 
> > This matches how you would get both sets of options to render with `-g`, 
> > which I think makes sense.
> Yeah, that sounds good. IIRC when we were using the joint codeview + DWARF 
> mode it was with the gcc-style Clang driver and not clang-cl, and we passed 
> `-gdwarf -gcodeview` or similar to enable it anyway. We don't need that 
> anymore though; @mstorsjo would know if there's anything on the MinGW side 
> that cares for it.
I'm not aware of mingw usecases of using both codeview and DWARF at the same 
time - I've occasionally mentioned that I know some people do use that, but I 
haven't actually guided anybody into doing it.

For mingw, as long as `-g -gcodeview` on the driver level generates codeview 
(and not DWARF) like it used to, mingw should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157794

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

[PATCH] D150646: [clang][X86] Add __cpuidex function to cpuid.h

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D150646#4581664 , @rnk wrote:

> I think we should be exposing the `__cpudex` builtin any time we are being 
> MSVC-like, not under `fms-compatibility`. Would that make things sensible 
> again?

I think that might sound reasonable - what do we do for other MSVC-specific 
builtins today?

In D150646#4581672 , @aidengrossman 
wrote:

> The main issue here is that there's no way to reliably detect whether the 
> built in is defined (can't check if a function is defined in the 
> preprocessor, preprocessor macro isn't exposed correctly in all 
> configurations), which ends up breaking some configurations.

That's indeed the issue

> I wouldn't be opposed to exposing things more often, but I'm fine with the 
> builtins being exposed under `-fms-extensions`, we just need to expose the 
> preprocessor macro when we expose the builtins.

As far as I can see, @rnk's suggestion is the opposite - to tie whether the 
builtin is enabled to the target only, not to `-fms-extensions`. I.e. mingw + 
`-fms-extensions` doesn't get the builtin. Then the preprocessor check for it 
would simply be `!defined(_MSC_VER)`.

Unfortunately, for Clang tests that invoke `clang -cc1` directly, with an MSVC 
target triple, don't get `_MSC_VER` defined automatically, only if they pass 
`-fms-compatibility-version=` (which the Driver usually passes), so in such 
test conditions, the main way of detecting MSVC right now is `defined(_WIN32) 
&& !defined(__MINGW32__)` which isn't very pretty either.

I played with a patch to make Clang always define `_MSC_VER` for MSVC targets, 
even if `-fms-compatibility-version=` wasn't set. That broke quite a few tests, 
since `_MSC_VER` unlocks lots of codepaths that only work when 
`-fms-extensions` is set (which the Driver usually does for MSVC targets, but 
usually isn't set in `%clang_cc1` tests). So if we want to default to defining 
`_MSC_VER` on the cc1 level without an explicit `-fms-compatibility-version=`, 
we'd also need to imply `-fms-extensions` for MSVC targets, which I guess would 
go against a lot of the driver/frontend logic split.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150646

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo marked an inline comment as done.
mstorsjo added a comment.

Thanks a lot for the guidance!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 549575.
mstorsjo added a comment.

Move the existing comment into the new if statement, add a comment to the new 
if. Add a comment to the second part of the testcase, which probably is good to 
keep anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | 
FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated 
for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns 
as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5777,9 +5778,13 @@
 DestIsVolatile = false;
   }
 
-  // If the value is offset in memory, apply the offset now.
-  Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-  CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  // An empty record can overlap other data (if declared with 
no_unique_address);
+  // omit the store for such types - as there is no actual data to store.
+  if (!isEmptyRecord(getContext(), RetTy, true)) {
+// If the value is offset in memory, apply the offset now.
+Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
+CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  }
 
   return convertTempToRValue(DestPtr, RetTy, SourceLocation());
 }


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include 

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 549517.
mstorsjo added a comment.

Updated to just check isEmptyRecord in EmitCall.

The second part of the test probably is kinda redundant/pointless at this 
point, and I guess the test comment needs updating too; can do that later when 
the implementation is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | 
FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated 
for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns 
as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5778,8 +5779,10 @@
   }
 
   // If the value is offset in memory, apply the offset now.
-  Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-  CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  if (!isEmptyRecord(getContext(), RetTy, true)) {
+Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
+CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  }
 
   return convertTempToRValue(DestPtr, RetTy, SourceLocation());
 }


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5778,8 +5779,10 @@
   }
 
   // If the value is offset in memory, apply the offset now.
-  Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-  CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  if (!isEmptyRecord(getContext(), RetTy, true)) {
+Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 549099.
mstorsjo added a comment.

Plumb the info from EmitInitializerForField through AggValueSlot and 
ReturnValueSlot to EmitCall.

The fields/variables could use better names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGValue.h
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp

Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGValue.h
===
--- clang/lib/CodeGen/CGValue.h
+++ clang/lib/CodeGen/CGValue.h
@@ -559,13 +559,16 @@
   /// them.
   bool SanitizerCheckedFlag : 1;
 
+  bool IsDummy : 1;
+
+public:
   AggValueSlot(Address Addr, Qualifiers Quals, bool DestructedFlag,
bool ObjCGCFlag, bool ZeroedFlag, bool AliasedFlag,
-   bool OverlapFlag, bool SanitizerCheckedFlag)
+   bool OverlapFlag, bool SanitizerCheckedFlag, bool IsDummy)
   : Addr(Addr), Quals(Quals), DestructedFlag(DestructedFlag),
 ObjCGCFlag(ObjCGCFlag), ZeroedFlag(ZeroedFlag),
 AliasedFlag(AliasedFlag), OverlapFlag(OverlapFlag),
-SanitizerCheckedFlag(SanitizerCheckedFlag) {}
+SanitizerCheckedFlag(SanitizerCheckedFlag), IsDummy(IsDummy) {}
 
 public:
   enum IsAliased_t { IsNotAliased, IsAliased };
@@ -574,6 +577,7 @@
   enum Overlap_t { DoesNotOverlap, MayOverlap };
   enum NeedsGCBarriers_t { DoesNotNeedGCBarriers, NeedsGCBarriers };
   enum IsSanitizerChecked_t { IsNotSanitizerChecked, IsSanitizerChecked };
+  enum IsDummy_t { IsNotADummySlot, IsADummySlot };
 
   /// ignored - Returns an aggregate value slot indicating that the
   /// aggregate value is being ignored.
@@ -592,27 +596,26 @@
   ///   for calling destructors on this object
   /// \param needsGC - true if the slot is potentially located
   ///   somewhere that ObjC GC calls should be emitted for
-  static AggValueSlot forAddr(Address addr,
-  Qualifiers quals,
-  IsDestructed_t isDestructed,
-  NeedsGCBarriers_t needsGC,
-  IsAliased_t isAliased,
-  Overlap_t mayOverlap,
-  IsZeroed_t isZeroed = IsNotZeroed,
-   IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) {
+  static AggValueSlot
+  forAddr(Address addr, Qualifiers quals, IsDestructed_t isDestructed,
+  NeedsGCBarriers_t needsGC, IsAliased_t isAliased,
+  Overlap_t mayOverlap, IsZeroed_t isZeroed = IsNotZeroed,
+  IsSanitizerChecked_t isChecked = IsNotSanitizerChecked,
+  IsDummy_t isDummy = IsNotADummySlot) {
 if (addr.isValid())
   addr.setKnownNonNull();
 return AggValueSlot(addr, quals, isDestructed, needsGC, isZeroed, isAliased,
-mayOverlap, isChecked);
+mayOverlap, isChecked, isDummy);
   }
 
   static AggValueSlot
   forLValue(const LValue , CodeGenFunction , IsDestructed_t isDestructed,
 NeedsGCBarriers_t needsGC, IsAliased_t isAliased,
 Overlap_t mayOverlap, IsZeroed_t isZeroed = IsNotZeroed,
-IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) {
+IsSanitizerChecked_t isChecked = IsNotSanitizerChecked,
+IsDummy_t isDummy = 

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

I think this makes sense in principle - however it does diverge from GCC. GCC 
also supports `-fms-extensions` (but I'm not sure if the set of extensions that 
are enabled by the option is an exact match between the two cases), but GCC 
doesn't expose any extra define in that mode. I don't foresee that it'll cause 
any concrete harm though...

Which toolchain is the original source for the name `_MSC_EXTENSIONS` - is it 
something that MSVC itself sets (always, or optionally?), or something that 
Clang itself has made up?

I tried including this patch in a nightly build run (building llvm-mingw and a 
handful of projects with it, including VLC as a large testcase), and it didn't 
trip up anything, so it seems mostly safe in that aspect at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157334

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D157332#4570290 , @crtrott wrote:

> Question: does this bug potentially affect code generation for 
> AMD/NVIDIA/Intel GPUs?

I believe the easiest way to test that is to try compiling `struct S {}; S 
ret() { return S(); }` into IR and checking the signature - if it returns void, 
the target should be immune to this bug, otherwise the bug probably is present.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D157332#4568341 , @efriedma wrote:

> I see what you're getting at here... but I don't think this works quite 
> right.  If the empty class has a non-trivial constructor, we have to pass the 
> correct "this" address to that constructor.  Usually a constructor for an 
> empty class won't do anything with that address, but it could theoretically 
> do something with it.

Hmm, I think I see. In this specific case, there's no constructor for the empty 
class invoked at all, we call the function `f()` which returns an `struct S` 
(which is empty). But if we'd remove the initialization of `y(f())` and add a 
constructor to `S()`, I see that we generate code that is indeed wrong in that 
aspect.

> In order to preserve the address in the cases we need it, we need a different 
> invariant: the handling for each aggregate expression in EmitAggExpr needs to 
> ensure it doesn't store anything to an empty class.  Which is unfortunately 
> more fragile than I'd like, but I can't think of a better approach.  This 
> check needs to happen further down the call stack.  Maybe put it in 
> CodeGenFunction::EmitCall.

Ok, I'll see if I can look further into that... (I don't have a huge amount of 
time for it at the moment, so if someone else with an interest in the area, 
e.g. @crtrott or @amyk want to dig into it, feel free!)




Comment at: clang/test/CodeGenCXX/ctor-empty-nounique.cpp:1
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+

cor3ntin wrote:
> This should probably get another run line for powerpc64le
Sure, I could do that. (Initially I had both, but thought people would consider 
it unnecessary duplication.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: aaron.ballman, efriedma.
Herald added subscribers: steven.zhang, pengfei.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: clang.

An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

The existing clang tests in CodeGenCXX/tail-padding.cpp contain
cases of fields that have no_unique_address on non-empty struct
members; the check for isEmptyRecord() is needed to retain the
previous behaviour in that test.

Fixes https://github.com/llvm/llvm-project/issues/64253, and
possibly https://github.com/llvm/llvm-project/issues/64077
and https://github.com/llvm/llvm-project/issues/64427 as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated 
for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns 
as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+
+// CHECK: %coerce = alloca %struct.S, align 1
+
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: %coerce.dive = getelementptr inbounds %struct.S, ptr %coerce, 
i32 0, i32 0
+// CHECK-NEXT: store i8 %call, ptr %coerce.dive, align 1
+// CHECK-NEXT: ret void
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -10,6 +10,7 @@
 //
 
//===--===//
 
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGDebugInfo.h"
@@ -701,6 +702,9 @@
 getOverlapForFieldInit(Field), AggValueSlot::IsNotZeroed,
 // Checks are made by the code that calls constructor.
 AggValueSlot::IsSanitizerChecked);
+if (Field->hasAttr() &&
+isEmptyRecord(getContext(), FieldType, true))
+  Slot = AggValueSlot::ignored();
 EmitAggExpr(Init, Slot);
 break;
   }


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+
+// CHECK: %coerce = alloca %struct.S, align 1
+
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: %coerce.dive = getelementptr inbounds %struct.S, ptr %coerce, i32 0, i32 0
+// CHECK-NEXT: store i8 %call, ptr %coerce.dive, align 1
+// CHECK-NEXT: ret void
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGDebugInfo.h"
@@ -701,6 +702,9 @@
 getOverlapForFieldInit(Field), AggValueSlot::IsNotZeroed,
 // Checks are made by the code that calls 

[PATCH] D157115: Revert "[clang][X86] Add __cpuidex function to cpuid.h"

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D157115#4561497 , @aaron.ballman 
wrote:

> Thank you for this! The revert LGTM, but please wait for @mstorsjo to confirm 
> this won't cause problems for MinGW folks.

I guess this is fine.

We should probably sync mingw-w64 and revert 
https://github.com/mingw-w64/mingw-w64/commit/2b6c9247613aa830374e3686e09d3b8d582a92a6
 after that. There's less rush in reverting it though (not many projects use 
`__cpuidex` so it's not so bad to not have it defined it at all - while having 
a double conflicting definition is fatal), but I guess I should update it at 
least once the 17.0.0 release gets closer and we see where we're settled (maybe 
bump the version threshold to 18 instead of 17, if we're somewhat sure we'll 
have it in main even if it's reverted in 17.x).

For mingw, I'd at least appreciate if we don't switch it back and forth further 
on the 17.x release branch after 17.0.0 is released.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157115

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

[PATCH] D61670: [RFC] [MinGW] Allow opting out from .refptr stubs

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added subscribers: alvinhochun, jeremyd2019, jacek, aaron.ballman.
mstorsjo added a comment.

I'm looking to pick this up again - hopefully @rnk has time to discuss what 
would be a good way forward.

So taking it from the top; both GCC and Clang generate `.refptr` stubs when 
referencing variables that might be autoimported. The exact circumstances 
differ a little bit though.

In GCC this is done by making GCC for Windows/x86_64 default to a medium code 
model, and under this code model, references to variables without a visible 
definition get indirection via a `.refptr` stub. This doesn't happen for i386. 
By building with `-mcmodel=small`, one can opt out of them.

In Clang, we generate such `.refptr` stubs with similar heuristics (variables 
without a visible definition), but we do it for all architectures. There are 
three different reasons for wanting to do this:

- Normally, when code references a variable (in the default code model), it is 
done with a 32 bit relative (or absolute) relocation. If the variable turns out 
to end up imported from a different DLL, in a 64 bit address space it may end 
up loaded too far away for a 32 bit relocation.
- If we don't use a `.refptr` stub, the mingw runtime pseudo relocation 
mechanism needs to patch the address at runtime. If this is referenced in a 
code section, it means that the code section needs to be mapped RWX while 
patching it. Changing the memory protection to RWX is generally undesireable 
(and is disallowed outright, within the UWP app model).
- The runtime pseudo relocation format works on N-bit relative or absolute 
addresses. With the x86 instruction encoding, this turns out to work fine - an 
instruction that refers to a different memory location generally is a couple 
bytes of instruction prefix, followed by a 32 bit relative or absolute address, 
easily patchable. In the case of ARM and AArch64, there are no such 
instructions, and loading e.g. a 32 bit immediate constant is often done by a 
pair of instructions, with the actual payload bits scattered throughout the 
instructions. The runtime pseudo relocation format obviously doesn't support 
patching this.

Therefore, with Clang we generate `.refptr` stubs on all architectures. However 
it would be good to be able to opt out from them. When the variables actually 
end up autoimported, LLD has got a couple neat tricks that makes the actual 
pseudo relocations go away in most cases (when it can alias the 
`.refptr.variable` stub with the `__imp_variable` IAT entry). But for the cases 
when the variable isn't autoimported, the `.refptr` stub has to be kept, and it 
produces larger/slower code and more data than necessary. And for low level 
projects like Wine, it might be desireable to be able to tune exactly what is 
done.

So within the Clang context, it is not entirely about range (where the code 
model might be a reasonable fit), but about whether to be prepared for 
autoimports or not.

Within Clang, it is handled by marking variables we know are in the same DLL as 
`dso_local`, while variables that might be autoimported don't get that flag. 
Within LLVM, references to variables that aren't marked `dso_local` get 
indirection via a `.refptr` stub.

Possible ways of handling it would be to invent a new flag for this purpose; 
e.g. `-fno-autoimport` (with the corresponding `-fautoimport` meaning the 
default mode). That makes the use fairly clear, but its role is also a bit 
unclear; there are linker flags `--disable-auto-import` and 
`--disable-runtime-pseudo-reloc` which disable either autoimports of all sorts, 
or only autoimports that end up with an actual pseudo reloc (allowing the 
zero-cost ones that are aliased to IAT entries).

If we make the flag only affect code generation, having it named 
`-fno-autoimport` when the linker still may do something different (ending up 
with a pseudo relocation in executable code, which possibly still works fine, 
just less elegant) is unclear. If we on the other hand make the same flag imply 
either of `--disable-auto-import` or `--disable-runtime-pseudo-reloc`, then 
that name is rather clear.

If we only make it affect code generation, something like `-fdso-local` or 
`-fno-refptr` might be more precise.

If we make the flag imply linker options, then it becomes much clearer to spot 
the error, if you enabled this but the code base still would need autoimports 
somewhere. (This has happened - see 
https://code.videolan.org/videolan/dav1d/-/merge_requests/1303#note_301859, 
https://code.videolan.org/videolan/dav1d/-/merge_requests/1349 and 
https://code.videolan.org/videolan/dav1d/-/merge_requests/1361.) If we make the 
flag only affect code generation, it becomes a more clear match for projects 
using `-mcmodel=small` with GCC today.

I'm open for opinions on how to name these options, @alvinhochun @mati865 
@jeremyd2019 and @jacek. Also requesting early guidance from @aaron.ballman.


Repository:
  rC Clang

CHANGES SINCE LAST 

[PATCH] D155540: [clangd] Remove extra dependancies for clangd

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a reviewer: sammccall.
mstorsjo added a subscriber: sammccall.
mstorsjo added a comment.

Thanks for looking at my suggestion! At this point, I'd defer the judgement to 
the code owner, @sammccall - whether it's nicer to have a tighter set of 
dependencies, or to reduce the risk of churn and accidentally breaking the 
shared libs build again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155540

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

[PATCH] D151188: [LLD][COFF] Add LLVM toolchain library paths by default.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Maybe add a release note for this too? It's rather significant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151188

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

[PATCH] D151188: [LLD][COFF] Add LLVM toolchain library paths by default.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo accepted this revision.
mstorsjo added a comment.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151188

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

[PATCH] D155111: [clangd] Fix build failures observed on build bots for missing libs

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

To clarify the issue - the kind of builds that seems to be broken is builds 
with `BUILD_SHARED_LIBS=ON`. The reason is that these libraries are needed is 
because the `clangd` target includes `$`, 
so all the dependencies of `clangDaemonTweaks` would need to be included here 
as well. Please include that in the commit message description. (Is there a way 
to pull in those instead of duplicating the list?)

This looks mostly ok to me, but it does add slightly more libraries than what's 
needed. As the list of libraries that now are linked into `clangdMain` is the 
list of libraries that previously was linked for the two components that now 
are `clangd` and `clangdMain`, so some of the dependencies only need to be 
moved, not duplicated.

A more minimal set of dependencies, which seems to link successfully with 
`BUILD_SHARED_LIBS=ON`, is achieved with this diff on top of current git main:

  diff --git a/clang-tools-extra/clangd/tool/CMakeLists.txt 
b/clang-tools-extra/clangd/tool/CMakeLists.txt
  index ddf9c2488819..6c21175d7687 100644
  --- a/clang-tools-extra/clangd/tool/CMakeLists.txt
  +++ b/clang-tools-extra/clangd/tool/CMakeLists.txt
  @@ -26,11 +26,7 @@ clang_target_link_libraries(clangdMain
 clangBasic
 clangFormat
 clangFrontend
  -  clangLex
  -  clangSema
 clangTooling
  -  clangToolingCore
  -  clangToolingRefactoring
 clangToolingSyntax
 )
   
  @@ -44,7 +40,20 @@ target_link_libraries(clangdMain
 ${CLANGD_XPC_LIBS}
 )
   
  +clang_target_link_libraries(clangd
  +  PRIVATE
  +  clangAST
  +  clangBasic
  +  clangLex
  +  clangSema
  +  clangToolingCore
  +  clangToolingRefactoring
  +  clangToolingSyntax
  +  )
  +
   target_link_libraries(clangd
 PRIVATE
 clangdMain
  +  clangDaemon
  +  clangdSupport
 )

Not sure if it's good hygiene to only link specifically to exactly those 
libraries that are needed and nothing else, or if it's just making things 
slightly more brittle?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155111

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D145302#4491973 , @glandium wrote:

> This broke building with `-DLLVM_LINK_LLVM_DYLIB=ON`:

I also ran into this; I pushed a fix in 
a20d57e83441a69fa2bab86593b18cc0402095d2 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

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

[PATCH] D154295: [Driver][MSVC] Support DWARF fission when using LTO on Windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo accepted this revision.
mstorsjo added subscribers: hans, rnk.
mstorsjo added a comment.
This revision is now accepted and ready to land.

I think this looks reasonable, unless e.g. @hans or @rnk would disagree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154295

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

[PATCH] D154176: [Driver][MSVC] Move lld specific flags before inputs

[Martin Storsjö via Phabricator via cfe-commits]

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

In D154176#4466230 , @HaohaiWen wrote:

> In D154176#4466190 , @mstorsjo 
> wrote:
>
>> In D154176#4466186 , @mstorsjo 
>> wrote:
>>
>>> Ok, thanks for clarifying. However I still don’t understand the “why” 
>>> aspect here. You’re writing
>>>
 so that lld specific flags can be append before inputs
>>>
>>> Are you planning on adding such flags in a later patch, position dependent 
>>> flags that need to be supplied before input files?
>>>
>>> Or does the change in order of command line arguments have an effect on the 
>>> behavior of the linker in this case?
>>
>> Ok, on second thought, I guess that parsing the vfsoverlay option before the 
>> inputs makes it have an effect where it didn’t before. Is that right? (Most 
>> flags in lld aren’t very order dependent since it just checks for any 
>> occurrence of a flag anywhere among the arguments.)
>
> I'm planing to add /dwodir to linker when user specify -gsplit-dwarf and 
> using lld and lto.
> The order does not matter.

Ok, thanks for clarifying that - since I didn't quite understand how this 
change would be needed for that option.

> I just want it looks more consistent and elegent since most flags are before 
> inputs so that user can easily look for inputs when using -###.

Ok, that sounds reasonable. Please clarify that intent in the commit message, 
and this seems ok to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154176

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

[PATCH] D154176: [Driver][MSVC] Move lld specific flags before inputs

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D154176#4466186 , @mstorsjo wrote:

> Ok, thanks for clarifying. However I still don’t understand the “why” aspect 
> here. You’re writing
>
>> so that lld specific flags can be append before inputs
>
> Are you planning on adding such flags in a later patch, position dependent 
> flags that need to be supplied before input files?
>
> Or does the change in order of command line arguments have an effect on the 
> behavior of the linker in this case?

Ok, on second thought, I guess that parsing the vfsoverlay option before the 
inputs makes it have an effect where it didn’t before. Is that right? (Most 
flags in lld aren’t very order dependent since it just checks for any 
occurrence of a flag anywhere among the arguments.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154176

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

[PATCH] D154176: [Driver][MSVC] Move lld specific flags before inputs

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Ok, thanks for clarifying. However I still don’t understand the “why” aspect 
here. You’re writing

> so that lld specific flags can be append before

inputs

Are you planning on adding such flags in a later patch, position dependent 
flags that need to be supplied before input files?

Or does the change in order of command line arguments have an effect on the 
behavior of the linker in this case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154176

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

[PATCH] D154176: [Driver][MSVC] Move lld specific flags before inputs

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Is this a NFC change, as a preparation for a separate change? In that case, 
please add an NFC tag to the subject - if not, please explain (and test) the 
expected behaviour change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154176

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

[PATCH] D154070: [lld/COFF] Add /dwodir to enable DWARF fission with LTO

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

No objection from me here, this seems straightforward. We should probably add a 
corresponding option in the lld mingw frontend too (as a separate patch later).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154070

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

[PATCH] D152785: [COFF] Support -gsplit-dwarf for COFF on Windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D152785#4438544 , @MaskRay wrote:

> CodeView is the default debug info format for COFF. Perhaps `-gsplit-dwarf` 
> should be rejected when the default `-gcodeview` is used, with a test.

IIRC some users do generate both CodeView and DWARF debug info at the same 
time, for some use cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152785

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

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This change triggers failed asserts when compiling code for at least arm and 
aarch64. It is reproducible with this reduced testcase:

  $ cat repro.c
  typedef long long a;
  typedef int b();
  int c, d;
  long e, f;
  short g, j;
  void *h;
  short i[];
  char k;
  a l, m, n;
  void o();
  int p();
  void r(b u) {
struct {
  a q;
  a s, t
} a;
e || p(c, d);
f = l = a.s;
for (; a.s;)
  if (p(c, a, , a))
a.t & &(a, k);
if (g)
  u(i, m, n, 0, h);
f = a.s;
for (; a.s;)
  a.t & &(c, a, k);
if (g && j)
  o(g);
  }
  int v(short *, long long, long long, int, void *) { r(v); } 
  $ clang -target aarch64-linux-gnu -c repro.c -g -O2
  clang: ../lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2334: virtual void 
llvm::DwarfDebug::endFunctionImpl(const llvm::MachineFunction*): Assertion 
`LScopes.getAbstractScopesList().size() == NumAbstractSubprograms && 
"getOrCreateAbstractScope() inserted an abstract subprogram scope"' failed. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D152856: [Driver] Allow warning for unclaimed TargetSpecific options

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: llvm/include/llvm/Option/Arg.h:53
+  /// This is used for generating an "argument unused" warning (without
+  /// clang::driver::options::TargetSpecific) or "unsupported option" error
+  /// (with TargetSpecific).

MaskRay wrote:
> mstorsjo wrote:
> > Technically, as this is a llvm level API, talking about clang specific 
> > behaviours here is a bit at the wrong level.
> Yes. LLVMOption was originally part of Clang and was moved to llvm/.
> 
> The "argument unused" string is also quite Clang-specific. There are other 
> stuff talking about Clang behaviors, e.g.  suggestValueCompletions, `Alias` 
> (`-finput-charset=utf-8`). I wish that this is not too bad.
Ok, fair enough - consider this remark withdrawn then. Even if mislayered, 
detailed comments explaining the reason for the field is better than no 
explanations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152856

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

[PATCH] D152856: [Driver] Allow warning for unclaimed TargetSpecific options

[Martin Storsjö via Phabricator via cfe-commits]

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

Looks great to me, thanks! (Didn't test it myself yet though.) Just one small 
nit.




Comment at: llvm/include/llvm/Option/Arg.h:53
+  /// This is used for generating an "argument unused" warning (without
+  /// clang::driver::options::TargetSpecific) or "unsupported option" error
+  /// (with TargetSpecific).

Technically, as this is a llvm level API, talking about clang specific 
behaviours here is a bit at the wrong level.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152856

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

[PATCH] D152318: [Fuchsia] Add llvm-strings to Fuchsia clang build

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D152318#4402493 , @haowei wrote:

> In D152318#4402234 , @mstorsjo 
> wrote:
>
>> FWIW, I'm curious about where you need `llvm-strings` in a MinGW setting. 
>> While it does match a GNU binutils tool, I'm kinda curious where it is 
>> needed.
>
> llvm-strings exists in the `output_dir/bin` from a local llvm-mingw build 
> using the scripts in your repo. So I think we might need it. I didn't dig 
> into it why it exists in the first place. The current effort is to replicate 
> the results. If it is not required I think there is no harm to added to 
> Fuchsia's Clang configuration.
>
> We are trying to build and setup a automated builder for llvm-mingw project 
> using Fuchsia's Clang toolchain (instead of a pinned version of LLVM from 
> llvm-mingw's build script) and using it to build some Windows host tools.

Ok, I see. Yeah there's not much effort in providing it. I tried digging in to 
see when I added it, and it seems I added it in 
https://github.com/mstorsjo/llvm-mingw/commit/55e8aef024faa83c17e18ece15c191bbc1ab7936,
 without any specific explanation why this particular tool was added, so I 
guess I added it mostly for completeness. In any case, it doesn't hurt to 
include it for consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152318

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

[PATCH] D152318: [Fuchsia] Add llvm-strings to Fuchsia clang build

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

FWIW, I'm curious about where you need `llvm-strings` in a MinGW setting. While 
it does match a GNU binutils tool, I'm kinda curious where it is needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152318

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

[PATCH] D148793: [clang-tidy] Implement an include-cleaner check.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D148793#4393385 , @mgorny wrote:

> My educated guess would be that `clangIncludeCleaner` is being linked via 
> `clang_target_link_libraries` while it's not part of `libclang-cpp`, so it 
> should go into regular `target_link_libraries`.

Yes, that does seem to do the trick. LGTM from my PoV if you can push such a 
fix, otherwise I can try to do it in a little while...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793

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

[PATCH] D148793: [clang-tidy] Implement an include-cleaner check.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D148793#4392788 , @mgorny wrote:

> I'm getting completely incomprehensible build errors on Gentoo from this

I'm also hitting this; it only seems to happen if building with 
`-DLLVM_LINK_LLVM_DYLIB=ON`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793

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

[PATCH] D151662: [clang] [test] Fix test failures due to -Wbuiltin-macro-redefined in MinGW mode

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo abandoned this revision.
mstorsjo added a comment.

Superseded by D151741 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151662

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

[PATCH] D151783: [clang] Use the appropriate definition when checking FunctionDecl::isInlineBuiltinDeclaration

[Martin Storsjö via Phabricator via cfe-commits]

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

LGTM, thanks!

I'm not familiar with the code in question to know whether this really is the 
right thing to do etc, but this fixes my testcase (both the reduced one and the 
full one).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151783

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

[PATCH] D151741: [Lex] Only warn on defining or undefining language-defined builtins

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Thanks! +1 from me, this does clear up all the issues I've had - it supersedes 
D151662  and makes the external patch to 
avoid doing `-U__STRICT_ANSI__` unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151741

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

[PATCH] D151661: [clang] [test] Narrow down an MSVC specific behaviour to only not covever MinGW

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG28b26b161c2f: [clang] [test] Narrow down an MSVC specific 
behaviour to only not covever MinGW (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151661

Files:
  clang/test/CXX/drs/dr9xx.cpp


Index: clang/test/CXX/drs/dr9xx.cpp
===
--- clang/test/CXX/drs/dr9xx.cpp
+++ clang/test/CXX/drs/dr9xx.cpp
@@ -92,7 +92,7 @@
 
 namespace dr977 { // dr977: yes
 enum E { e = E() };
-#ifndef _WIN32
+#if !defined(_WIN32) || defined(__MINGW32__)
 // expected-error@-2 {{invalid use of incomplete type 'E'}}
 // expected-note@-3 {{definition of 'dr977::E' is not complete until the 
closing '}'}}
 #endif


Index: clang/test/CXX/drs/dr9xx.cpp
===
--- clang/test/CXX/drs/dr9xx.cpp
+++ clang/test/CXX/drs/dr9xx.cpp
@@ -92,7 +92,7 @@
 
 namespace dr977 { // dr977: yes
 enum E { e = E() };
-#ifndef _WIN32
+#if !defined(_WIN32) || defined(__MINGW32__)
 // expected-error@-2 {{invalid use of incomplete type 'E'}}
 // expected-note@-3 {{definition of 'dr977::E' is not complete until the closing '}'}}
 #endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Restrict Inline Builtin to non-static, non-odr linkage

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D148723#4381414 , 
@serge-sans-paille wrote:

> In D148723#4380165 , @mstorsjo 
> wrote:
>
>> This causes failed asserts with `_FORTIFY_SOURCE` with the mingw-w64 
>> headers. Here's a reduced reproducer:
>>
>>   $ cat reduced.c
>>   typedef unsigned int size_t;
>>   
>>   void *memcpy(void *_Dst, const void *_Src, size_t _Size);
>>   
>>   extern __inline__ __attribute__((__always_inline__, __gnu_inline__)) 
>> __attribute__((__artificial__)) 
>>   void *memcpy(void *__dst, const void *__src, size_t __n) 
>>   {
>> return __builtin___memcpy_chk(__dst, __src, __n, 
>> __builtin_object_size((__dst), ((0) > 0) && (2 > 1))); 
>>   } 
>>   
>>   void *memcpy(void *_Dst, const void *_Src, size_t _Size);
>>   
>>   char *a, *b;
>>   void func(void) {
>>   memcpy(a, b, 42);
>>   }
>>   $ clang -target i686-w64-mingw32 -c reduced.c -O2
>>   clang: ../../clang/lib/AST/Decl.cpp:3763: bool 
>> clang::FunctionDecl::isInlineDefinitionExternallyVisible() const: Assertion 
>> `(doesThisDeclarationHaveABody() || willHaveBody() || hasAttr()) 
>> && "Must be a function definition"' failed.
>>
>> Here, the second declaration of the regular extern version of the function 
>> is what is triggering the issue.
>>
>> Can we revert this to unbreak my builds until we have a fix?
>
> I can't reproduce locally. Can you check your reproducer on current main?

Still reproduces at d81ce04587c006b6731198956c522c93d0df1050 
 (and the 
commit you referenced on irc). Did you use the exact command line from the 
repro? Note that it uses a 32 bit target triple - with a 64 bit target, you 
need to change the definition of `size_t`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148723

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

[PATCH] D144654: [Lex] Warn when defining or undefining any builtin macro

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

FWIW, I've also run into noisy warnings caused by this, in a few places. 
D151662  fixes a bunch of clang's tests when 
run in MinGW configurations, and 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20230526104837.20594-1-mar...@martin.st/
 tries to avoid warnings due to `-U__STRICT_ANSI__` in an external project.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144654

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

[PATCH] D148723: [clang] Restrict Inline Builtin to non-static, non-odr linkage

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This causes failed asserts with `_FORTIFY_SOURCE` with the mingw-w64 headers. 
Here's a reduced reproducer:

  $ cat reduced.c
  typedef unsigned int size_t;
  
  void *memcpy(void *_Dst, const void *_Src, size_t _Size);
  
  extern __inline__ __attribute__((__always_inline__, __gnu_inline__)) 
__attribute__((__artificial__)) 
  void *memcpy(void *__dst, const void *__src, size_t __n) 
  {
return __builtin___memcpy_chk(__dst, __src, __n, 
__builtin_object_size((__dst), ((0) > 0) && (2 > 1))); 
  } 
  
  void *memcpy(void *_Dst, const void *_Src, size_t _Size);
  
  char *a, *b;
  void func(void) {
  memcpy(a, b, 42);
  }
  $ clang -target i686-w64-mingw32 -c reduced.c -O2
  clang: ../../clang/lib/AST/Decl.cpp:3763: bool 
clang::FunctionDecl::isInlineDefinitionExternallyVisible() const: Assertion 
`(doesThisDeclarationHaveABody() || willHaveBody() || hasAttr()) && 
"Must be a function definition"' failed.

Here, the second declaration of the regular extern version of the function is 
what is triggering the issue.

Can we revert this to unbreak my builds until we have a fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148723

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

[PATCH] D151661: [clang] [test] Narrow down an MSVC specific behaviour to only not covever MinGW

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D151661#4379958 , @Endill wrote:

> Thank you!
> I've asked Aaron what is the best way to check for MSVC-like triple before 
> relanding this test. So today we both learned.

Generally, the canonical way to check for it is `#ifdef _MSC_VER` - however in 
the `%clang_cc1` tests, it's not automatically defined. This, because the 
driver heuristically tries to guess a MSVC version to mimic, which it then 
passes as a version number to `clang -cc1` with `-fms-compatibility-version` or 
similar. When invoking `clang -cc1` manually, this doesn't get set and 
`_MSC_VER` is undefined. (I'm considering changing code to make sure that 
`_MSC_VER` always is defined in such cases too, even to a dummy version number 
.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151661

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

[PATCH] D150646: [clang][X86] Add __cpuidex function to cpuid.h

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D150646#4379543 , @glandium wrote:

> There seem to still be two problems with this change, with mingw:
>
> - with `-fms-extensions`:
>
>   echo '#include "cpuid.h"' | ./clang/bin/clang++ 
> --target=x86_64-w64-windows-gnu -xc++ - -fms-extensions
>   In file included from :1:
>   /tmp/clang/lib/clang/17/include/cpuid.h:334:22: error: definition of 
> builtin function '__cpuidex'
>   static __inline void __cpuidex (int __cpu_info[4], int __leaf, int 
> __subleaf)
>^
>   1 error generated.

Ouch - this one is tricky, as `-fms-extensions` doesn't seem to set any define 
(it doesn't set `_MSC_EXTENSIONS` at least - and I don't know if it would 
unlock surprises all around if it started doing that?); the main noticable 
difference in the output of `clang -E -dM - < /dev/null` seems to be that 
`__stdcall` and `__cdecl` and other calling conventions no longer are defines. 
But that's a rather brittle thing to check against.

> - conflict with mingw headers without the extensions:
>
>   $ echo '#include "intrin.h"' | ./clang/bin/x86_64-w64-mingw32-clang++ -xc++ 
> -
>   In file included from :1:
>   In file included from /tmp/clang/lib/clang/17/include/intrin.h:12:
>   In file included from 
> /tmp/clang/bin/../x86_64-w64-mingw32/include/intrin.h:70:
>   /tmp/clang/lib/clang/17/include/cpuid.h:334:22: error: static declaration 
> of '__cpuidex' follows non-static declaration
>   static __inline void __cpuidex (int __cpu_info[4], int __leaf, int 
> __subleaf)
>^
>   /tmp/clang/bin/../x86_64-w64-mingw32/include/psdk_inc/intrin-impl.h:2031:6: 
> note: previous definition is here
>   void __cpuidex(int CPUInfo[4], int function_id, int subfunction_id) {
>^
>   1 error generated.
>
> Although, for the latter, this was "fixed" in 
> https://sourceforge.net/p/mingw-w64/mingw-w64/ci/2b6c9247613aa830374e3686e09d3b8d582a92a6/

Regarding "fixed" - do you see any better way of fixing it there? As it's a 
static inline function, there's no simple way of knowing whether it was already 
defined or not; I went with the same pattern used for the existing version 
check for GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150646

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

[PATCH] D151662: [clang] [test] Fix test failures due to -Wbuiltin-macro-redefined in MinGW mode

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: john.brawn, aaron.ballman.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: clang.

These tests undefine __declspec or __stdcall, both which are
builtin macros in MinGW mode. Therefore, build those cases with
-Wno-builtin-macro-redefined.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151662

Files:
  clang/test/PCH/single-token-macro.c
  clang/test/Rewriter/objc-modern-boxing.mm
  clang/test/Rewriter/rewrite-byref-in-nested-blocks.mm
  clang/test/Rewriter/rewrite-modern-block-consts.mm
  clang/test/Rewriter/rewrite-modern-block.mm
  clang/test/Rewriter/rewrite-modern-default-property-synthesis.mm
  clang/test/Rewriter/rewrite-modern-extern-c-func-decl.mm
  clang/test/Rewriter/rewrite-modern-ivar-access.mm
  clang/test/Rewriter/rewrite-modern-ivars-1.mm
  clang/test/Rewriter/rewrite-modern-private-ivars.mm
  clang/test/Rewriter/rewrite-modern-struct-ivar-1.mm
  clang/test/Rewriter/rewrite-modern-struct-ivar.mm
  clang/test/Rewriter/rewrite-rewritten-initializer.mm

Index: clang/test/Rewriter/rewrite-rewritten-initializer.mm
===
--- clang/test/Rewriter/rewrite-rewritten-initializer.mm
+++ clang/test/Rewriter/rewrite-rewritten-initializer.mm
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 -x objective-c++ -Wno-return-type -fblocks -fms-extensions -rewrite-objc -fobjc-runtime=macosx-fragile-10.5 %s -o %t-rw.cpp
-// RUN: %clang_cc1 -fsyntax-only -Wno-address-of-temporary -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw.cpp
+// RUN: %clang_cc1 -fsyntax-only -Wno-address-of-temporary -Wno-builtin-macro-redefined -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw.cpp
 // RUN: %clang_cc1 -x objective-c++ -Wno-return-type -fblocks -fms-extensions -rewrite-objc %s -o %t-rw-modern.cpp
-// RUN: %clang_cc1 -fsyntax-only -Werror -Wno-address-of-temporary -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw-modern.cpp
+// RUN: %clang_cc1 -fsyntax-only -Werror -Wno-address-of-temporary -Wno-builtin-macro-redefined -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw-modern.cpp
 // radar 7669784
 
 typedef unsigned long size_t;
Index: clang/test/Rewriter/rewrite-modern-struct-ivar.mm
===
--- clang/test/Rewriter/rewrite-modern-struct-ivar.mm
+++ clang/test/Rewriter/rewrite-modern-struct-ivar.mm
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 -E %s -o %t.mm
 // RUN: %clang_cc1 -fblocks -rewrite-objc -fms-extensions %t.mm -o %t-rw.cpp 
 // RUN: FileCheck --input-file=%t-rw.cpp %s
-// RUN: %clang_cc1 -fsyntax-only -Werror -Wno-address-of-temporary -Wno-c++11-narrowing -std=c++11 -D"Class=void*" -D"id=void*" -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw.cpp
+// RUN: %clang_cc1 -fsyntax-only -Werror -Wno-address-of-temporary -Wno-c++11-narrowing -Wno-builtin-macro-redefined -std=c++11 -D"Class=void*" -D"id=void*" -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw.cpp
 
 struct S {
 int i1;
Index: clang/test/Rewriter/rewrite-modern-struct-ivar-1.mm
===
--- clang/test/Rewriter/rewrite-modern-struct-ivar-1.mm
+++ clang/test/Rewriter/rewrite-modern-struct-ivar-1.mm
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fblocks -rewrite-objc -fms-extensions %s -o %t-rw.cpp
-// RUN: %clang_cc1 -Werror -fsyntax-only -Wno-address-of-temporary -Wno-c++11-narrowing -std=c++11 -D"Class=void*" -D"id=void*" -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw.cpp
+// RUN: %clang_cc1 -Werror -fsyntax-only -Wno-address-of-temporary -Wno-c++11-narrowing -Wno-builtin-macro-redefined -std=c++11 -D"Class=void*" -D"id=void*" -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw.cpp
 // rdar://11323187
 
 typedef unsigned long NSUInteger;
Index: clang/test/Rewriter/rewrite-modern-private-ivars.mm
===
--- clang/test/Rewriter/rewrite-modern-private-ivars.mm
+++ clang/test/Rewriter/rewrite-modern-private-ivars.mm
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fblocks -rewrite-objc -fms-extensions %s -o %t-rw.cpp
-// RUN: %clang_cc1 -Werror -fsyntax-only -Wno-address-of-temporary -Wno-c++11-narrowing -std=c++11 -D"Class=void*" -D"id=void*" -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw.cpp
+// RUN: %clang_cc1 -Werror -fsyntax-only -Wno-address-of-temporary -Wno-c++11-narrowing -Wno-builtin-macro-redefined -std=c++11 -D"Class=void*" -D"id=void*" -D"SEL=void*" -U__declspec -D"__declspec(X)=" %t-rw.cpp
 // rdar://11351299
 
 struct Q {
Index: clang/test/Rewriter/rewrite-modern-ivars-1.mm
===
--- clang/test/Rewriter/rewrite-modern-ivars-1.mm
+++ clang/test/Rewriter/rewrite-modern-ivars-1.mm
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -x objective-c++ -Wno-return-type -fblocks -fms-extensions -rewrite-objc %s -o %t-rw.cpp
-// RUN: %clang_cc1 -fsyntax-only -Werror 

[PATCH] D151661: [clang] [test] Narrow down an MSVC specific behaviour to only not covever MinGW

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: Endill, hans, aaron.ballman.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: clang.

This uses the same logic as in c2b256a990590dc8b69930259650cfeb085add03 
;
we can't check defined(_MSC_VER) invoked as %clang_cc1, therefore
check for !defined(__MINGW32__) instead.

This fixes the same issue in a new testcase that was added after this
issue was fixed last time in c2b256a990590dc8b69930259650cfeb085add03 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151661

Files:
  clang/test/CXX/drs/dr9xx.cpp


Index: clang/test/CXX/drs/dr9xx.cpp
===
--- clang/test/CXX/drs/dr9xx.cpp
+++ clang/test/CXX/drs/dr9xx.cpp
@@ -92,7 +92,7 @@
 
 namespace dr977 { // dr977: yes
 enum E { e = E() };
-#ifndef _WIN32
+#if !defined(_WIN32) || defined(__MINGW32__)
 // expected-error@-2 {{invalid use of incomplete type 'E'}}
 // expected-note@-3 {{definition of 'dr977::E' is not complete until the 
closing '}'}}
 #endif


Index: clang/test/CXX/drs/dr9xx.cpp
===
--- clang/test/CXX/drs/dr9xx.cpp
+++ clang/test/CXX/drs/dr9xx.cpp
@@ -92,7 +92,7 @@
 
 namespace dr977 { // dr977: yes
 enum E { e = E() };
-#ifndef _WIN32
+#if !defined(_WIN32) || defined(__MINGW32__)
 // expected-error@-2 {{invalid use of incomplete type 'E'}}
 // expected-note@-3 {{definition of 'dr977::E' is not complete until the closing '}'}}
 #endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151620: [clang-repl] Fix REPL_EXTERNAL_VISIBILITY and building libclang-cpp.dll for MinGW configurations

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG592e935e115f: [clang-repl] Fix REPL_EXTERNAL_VISIBILITY and 
building libclang-cpp.dll for… (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151620

Files:
  clang/include/clang/Interpreter/Value.h
  clang/tools/clang-shlib/CMakeLists.txt


Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -53,3 +53,11 @@
 if (NOT APPLE AND NOT MINGW)
   target_link_options(clang-cpp PRIVATE LINKER:-Bsymbolic-functions)
 endif()
+if (MINGW OR CYGWIN)
+  # The clang-cpp DLL is supposed to export all symbols (except for ones
+  # that are explicitly hidden). Normally, this is what happens anyway, but
+  # if there are symbols that are marked explicitly as dllexport, we'd only
+  # export them and nothing else. Therefore, add --export-all-symbols to
+  # make sure we export all symbols despite potential dllexports.
+  target_link_options(clang-cpp PRIVATE LINKER:--export-all-symbols)
+endif()
Index: clang/include/clang/Interpreter/Value.h
===
--- clang/include/clang/Interpreter/Value.h
+++ clang/include/clang/Interpreter/Value.h
@@ -52,18 +52,24 @@
 class Interpreter;
 class QualType;
 
-#if __has_attribute(visibility) && 
\
-(!(defined(_WIN32) || defined(__CYGWIN__)) ||  
\
- (defined(__MINGW32__) && defined(__clang__)))
+#if defined(_WIN32)
+// REPL_EXTERNAL_VISIBILITY are symbols that we need to be able to locate
+// at runtime. On Windows, this requires them to be exported from any of the
+// modules loaded at runtime. Marking them as dllexport achieves this; both
+// for DLLs (that normally export symbols as part of their interface) and for
+// EXEs (that normally don't export anything).
+// For a build with libclang-cpp.dll, this doesn't make any difference - the
+// functions would have been exported anyway. But for cases when these are
+// statically linked into an EXE, it makes sure that they're exported.
+#define REPL_EXTERNAL_VISIBILITY __declspec(dllexport)
+#elif __has_attribute(visibility)
 #if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS)
 #define REPL_EXTERNAL_VISIBILITY __attribute__((visibility("default")))
 #else
 #define REPL_EXTERNAL_VISIBILITY
 #endif
 #else
-#if defined(_WIN32)
-#define REPL_EXTERNAL_VISIBILITY __declspec(dllexport)
-#endif
+#define REPL_EXTERNAL_VISIBILITY
 #endif
 
 #define REPL_BUILTIN_TYPES 
\


Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -53,3 +53,11 @@
 if (NOT APPLE AND NOT MINGW)
   target_link_options(clang-cpp PRIVATE LINKER:-Bsymbolic-functions)
 endif()
+if (MINGW OR CYGWIN)
+  # The clang-cpp DLL is supposed to export all symbols (except for ones
+  # that are explicitly hidden). Normally, this is what happens anyway, but
+  # if there are symbols that are marked explicitly as dllexport, we'd only
+  # export them and nothing else. Therefore, add --export-all-symbols to
+  # make sure we export all symbols despite potential dllexports.
+  target_link_options(clang-cpp PRIVATE LINKER:--export-all-symbols)
+endif()
Index: clang/include/clang/Interpreter/Value.h
===
--- clang/include/clang/Interpreter/Value.h
+++ clang/include/clang/Interpreter/Value.h
@@ -52,18 +52,24 @@
 class Interpreter;
 class QualType;
 
-#if __has_attribute(visibility) && \
-(!(defined(_WIN32) || defined(__CYGWIN__)) ||  \
- (defined(__MINGW32__) && defined(__clang__)))
+#if defined(_WIN32)
+// REPL_EXTERNAL_VISIBILITY are symbols that we need to be able to locate
+// at runtime. On Windows, this requires them to be exported from any of the
+// modules loaded at runtime. Marking them as dllexport achieves this; both
+// for DLLs (that normally export symbols as part of their interface) and for
+// EXEs (that normally don't export anything).
+// For a build with libclang-cpp.dll, this doesn't make any difference - the
+// functions would have been exported anyway. But for cases when these are
+// statically linked into an EXE, it makes sure that they're exported.
+#define REPL_EXTERNAL_VISIBILITY __declspec(dllexport)
+#elif __has_attribute(visibility)
 #if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS)
 #define REPL_EXTERNAL_VISIBILITY __attribute__((visibility("default")))
 #else
 #define REPL_EXTERNAL_VISIBILITY
 #endif
 

[PATCH] D151620: [clang-repl] Fix REPL_EXTERNAL_VISIBILITY and building libclang-cpp.dll for MinGW configurations

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: junaire, alvinhochun, mati865.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a subscriber: bd1976llvm.
Herald added a project: clang.

This fixes two issues that are observed after
5111286f06e1e10f24745007a45a830760f1790c 
:

For builds with GCC with LLVM_LINK_LLVM_DYLIB=ON, we previously got
build errors, as libclang-cpp.dll suddenly only contained the
functions that were marked dllexport via REPL_EXTERNAL_VISIBILITY,
instead of all symbols as expected.

For MinGW builds with Clang, building previously succeeded (as it
used either the __attribute__((visibility("default"))) annotation or
nothing at all), and the functions were exported from libclang-cpp.dll
if that was built, but the unit test failed (as neither of those cases
made the functions exported from an EXE).

Don't use the visibility attributes on MinGW targets for these purposes;
setting default visibility only makes a difference if building with
e.g. -fvisibility=hidden, but it doesn't make the symbols exported
from an EXE.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151620

Files:
  clang/include/clang/Interpreter/Value.h
  clang/tools/clang-shlib/CMakeLists.txt


Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -53,3 +53,11 @@
 if (NOT APPLE AND NOT MINGW)
   target_link_options(clang-cpp PRIVATE LINKER:-Bsymbolic-functions)
 endif()
+if (MINGW OR CYGWIN)
+  # The clang-cpp DLL is supposed to export all symbols (except for ones
+  # that are explicitly hidden). Normally, this is what happens anyway, but
+  # if there are symbols that are marked explicitly as dllexport, we'd only
+  # export them and nothing else. Therefore, add --export-all-symbols to
+  # make sure we export all symbols despite potential dllexports.
+  target_link_options(clang-cpp PRIVATE LINKER:--export-all-symbols)
+endif()
Index: clang/include/clang/Interpreter/Value.h
===
--- clang/include/clang/Interpreter/Value.h
+++ clang/include/clang/Interpreter/Value.h
@@ -52,18 +52,24 @@
 class Interpreter;
 class QualType;
 
-#if __has_attribute(visibility) && 
\
-(!(defined(_WIN32) || defined(__CYGWIN__)) ||  
\
- (defined(__MINGW32__) && defined(__clang__)))
+#if defined(_WIN32)
+// REPL_EXTERNAL_VISIBILITY are symbols that we need to be able to locate
+// at runtime. On Windows, this requires them to be exported from any of the
+// modules loaded at runtime. Marking them as dllexport achieves this; both
+// for DLLs (that normally export symbols as part of their interface) and for
+// EXEs (that normally don't export anything).
+// For a build with libclang-cpp.dll, this doesn't make any difference - the
+// functions would have been exported anyway. But for cases when these are
+// statically linked into an EXE, it makes sure that they're exported.
+#define REPL_EXTERNAL_VISIBILITY __declspec(dllexport)
+#elif __has_attribute(visibility)
 #if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS)
 #define REPL_EXTERNAL_VISIBILITY __attribute__((visibility("default")))
 #else
 #define REPL_EXTERNAL_VISIBILITY
 #endif
 #else
-#if defined(_WIN32)
-#define REPL_EXTERNAL_VISIBILITY __declspec(dllexport)
-#endif
+#define REPL_EXTERNAL_VISIBILITY
 #endif
 
 #define REPL_BUILTIN_TYPES 
\


Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -53,3 +53,11 @@
 if (NOT APPLE AND NOT MINGW)
   target_link_options(clang-cpp PRIVATE LINKER:-Bsymbolic-functions)
 endif()
+if (MINGW OR CYGWIN)
+  # The clang-cpp DLL is supposed to export all symbols (except for ones
+  # that are explicitly hidden). Normally, this is what happens anyway, but
+  # if there are symbols that are marked explicitly as dllexport, we'd only
+  # export them and nothing else. Therefore, add --export-all-symbols to
+  # make sure we export all symbols despite potential dllexports.
+  target_link_options(clang-cpp PRIVATE LINKER:--export-all-symbols)
+endif()
Index: clang/include/clang/Interpreter/Value.h
===
--- clang/include/clang/Interpreter/Value.h
+++ clang/include/clang/Interpreter/Value.h
@@ -52,18 +52,24 @@
 class Interpreter;
 class QualType;
 
-#if __has_attribute(visibility) && \
-(!(defined(_WIN32) || defined(__CYGWIN__)) ||  \
- (defined(__MINGW32__) && defined(__clang__)))

[PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: libunwind/src/CMakeLists.txt:28-35
 
 # See add_asm_sources() in compiler-rt for explanation of this workaround.
 # CMake doesn't work correctly with assembly on AIX. Workaround by compiling
 # as C files as well.
 if((APPLE AND CMAKE_VERSION VERSION_LESS 3.19) OR
-   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17) OR
-   (${CMAKE_SYSTEM_NAME} MATCHES "AIX"))
+   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17))
   set_source_files_properties(${LIBUNWIND_ASM_SOURCES} PROPERTIES LANGUAGE C)

h-vetinari wrote:
> Shouldn't it be possible to remove that entire block (as it only fires for 
> CMake <3.20)?
The intention was to do such cleanups in a later patch, as mentioned in the 
original commit message in https://reviews.llvm.org/D144509. (I guess it would 
be good to bring the original commit message along with the reland attempts 
too.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151344

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

[PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D144509#4349051 , @hans wrote:

> In D144509#4347562 , @glandium 
> wrote:
>
>> FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c 
>>  is 
>> causing problems on Windows compiler-rt for some reason I haven't identified 
>> yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is 
>> actually altering its behavior, which I wouldn't have expected...
>
> It is indeed unfortunate that this seems to come with such a fundamental 
> behavior change. (we already have 
> https://github.com/llvm/llvm-project/issues/62719) Do you have a repro for 
> the issue you're seeing?
>
> Since the breakages are piling up, perhaps it's time to revert this.

Unfortunately, much of this is hard to sort out without actually trying to 
patch forward on main, since many of these issues don't show up easily in e.g. 
premerge testing or similar (the patch has been tried many times for months 
before in that form), so I'd appreciate if we'd give it a little more time to 
actually try to sort out the issues...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D144509#4347562 , @glandium wrote:

> FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c 
>  is 
> causing problems on Windows compiler-rt for some reason I haven't identified 
> yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is 
> actually altering its behavior, which I wouldn't have expected...

See D150688  - I believe that might fix the 
issue you're seeing, as that one mentions compiler-rt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc2b256a99059: Reapply [clang] [test] Narrow down MSVC 
specific behaviours from any windows… (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149997

Files:
  clang/test/C/drs/dr1xx.c
  clang/test/Driver/experimental-library-flag.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp


Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -5,11 +5,11 @@
 // Should not crash.
 template 
 class __attribute__((trivial_abi)) a { a(a &&); };
-#ifdef _WIN64
-// On Windows, to be trivial-for-calls, an object must be trivially copyable.
+#if defined(_WIN64) && !defined(__MINGW32__)
+// On Windows/MSVC, to be trivial-for-calls, an object must be trivially 
copyable.
 // (And it is only trivially relocatable, currently, if it is trivial for 
calls.)
 // In this case, it is suppressed by an explicitly defined move constructor.
-// Similar concerns apply to later tests that have #ifdef _WIN64.
+// Similar concerns apply to later tests that have #if defined(_WIN64) && 
!defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(a), "");
 #else
 static_assert(__is_trivially_relocatable(a), "");
@@ -137,7 +137,7 @@
   CopyDeleted(const CopyDeleted &) = delete;
   CopyDeleted(CopyDeleted &&) = default;
 };
-#ifdef _WIN64
+#if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(CopyDeleted), "");
 #else
 static_assert(__is_trivially_relocatable(CopyDeleted), "");
@@ -163,7 +163,7 @@
 struct __attribute__((trivial_abi)) S20 {
   int & // a member of rvalue reference type deletes the copy constructor.
 };
-#ifdef _WIN64
+#if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(S20), "");
 #else
 static_assert(__is_trivially_relocatable(S20), "");
Index: clang/test/Driver/experimental-library-flag.cpp
===
--- clang/test/Driver/experimental-library-flag.cpp
+++ clang/test/Driver/experimental-library-flag.cpp
@@ -1,6 +1,6 @@
 // On some platforms, -stdlib=libc++ is currently ignored, so 
-lc++experimental is not added.
 // Once -stdlib=libc++ works on those, this XFAIL can be removed.
-// XFAIL: target={{.*-windows.*}}, target={{.*-(ps4|ps5)}}
+// XFAIL: target={{.*-windows-msvc.*}}, target={{.*-(ps4|ps5)}}
 
 // For some reason, this fails with a core dump on AIX. This needs to be 
investigated.
 // UNSUPPORTED: target={{.*}}-aix{{.*}}
Index: clang/test/C/drs/dr1xx.c
===
--- clang/test/C/drs/dr1xx.c
+++ clang/test/C/drs/dr1xx.c
@@ -235,7 +235,7 @@
 * type at this point.
 */
 Val = sizeof(enum E)
-#ifndef _WIN32
+#if !defined(_WIN32) || defined(__MINGW32__)
 /* expected-error@-2 {{invalid application of 'sizeof' to an incomplete 
type 'enum E'}} */
 /* expected-note@-12 {{definition of 'enum E' is not complete until the 
closing '}'}} */
 #endif


Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -5,11 +5,11 @@
 // Should not crash.
 template 
 class __attribute__((trivial_abi)) a { a(a &&); };
-#ifdef _WIN64
-// On Windows, to be trivial-for-calls, an object must be trivially copyable.
+#if defined(_WIN64) && !defined(__MINGW32__)
+// On Windows/MSVC, to be trivial-for-calls, an object must be trivially copyable.
 // (And it is only trivially relocatable, currently, if it is trivial for calls.)
 // In this case, it is suppressed by an explicitly defined move constructor.
-// Similar concerns apply to later tests that have #ifdef _WIN64.
+// Similar concerns apply to later tests that have #if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(a), "");
 #else
 static_assert(__is_trivially_relocatable(a), "");
@@ -137,7 +137,7 @@
   CopyDeleted(const CopyDeleted &) = delete;
   CopyDeleted(CopyDeleted &&) = default;
 };
-#ifdef _WIN64
+#if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(CopyDeleted), "");
 #else
 static_assert(__is_trivially_relocatable(CopyDeleted), "");
@@ -163,7 +163,7 @@
 struct __attribute__((trivial_abi)) S20 {
   int & // a member of rvalue reference type deletes the copy constructor.
 };
-#ifdef _WIN64
+#if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(S20), "");
 #else
 static_assert(__is_trivially_relocatable(S20), "");
Index: clang/test/Driver/experimental-library-flag.cpp
===
--- 

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 521781.
mstorsjo added a comment.

Updated to check for `defined(_WIN32) && !defined(__MINGW32__)`. It's a kinda 
ugly way of checking for an MSVC-like environment specifically (when the MSVC 
mode is the exception compared with the rest), but neither `_MSC_VER` or 
`_MSC_EXTENSIONS` are defined by default in a `clang -cc1` invocation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149997

Files:
  clang/test/C/drs/dr1xx.c
  clang/test/Driver/experimental-library-flag.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp


Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -5,11 +5,11 @@
 // Should not crash.
 template 
 class __attribute__((trivial_abi)) a { a(a &&); };
-#ifdef _WIN64
-// On Windows, to be trivial-for-calls, an object must be trivially copyable.
+#if defined(_WIN64) && !defined(__MINGW32__)
+// On Windows/MSVC, to be trivial-for-calls, an object must be trivially 
copyable.
 // (And it is only trivially relocatable, currently, if it is trivial for 
calls.)
 // In this case, it is suppressed by an explicitly defined move constructor.
-// Similar concerns apply to later tests that have #ifdef _WIN64.
+// Similar concerns apply to later tests that have #if defined(_WIN64) && 
!defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(a), "");
 #else
 static_assert(__is_trivially_relocatable(a), "");
@@ -137,7 +137,7 @@
   CopyDeleted(const CopyDeleted &) = delete;
   CopyDeleted(CopyDeleted &&) = default;
 };
-#ifdef _WIN64
+#if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(CopyDeleted), "");
 #else
 static_assert(__is_trivially_relocatable(CopyDeleted), "");
@@ -163,7 +163,7 @@
 struct __attribute__((trivial_abi)) S20 {
   int & // a member of rvalue reference type deletes the copy constructor.
 };
-#ifdef _WIN64
+#if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(S20), "");
 #else
 static_assert(__is_trivially_relocatable(S20), "");
Index: clang/test/Driver/experimental-library-flag.cpp
===
--- clang/test/Driver/experimental-library-flag.cpp
+++ clang/test/Driver/experimental-library-flag.cpp
@@ -1,6 +1,6 @@
 // On some platforms, -stdlib=libc++ is currently ignored, so 
-lc++experimental is not added.
 // Once -stdlib=libc++ works on those, this XFAIL can be removed.
-// XFAIL: target={{.*-windows.*}}, target={{.*-(ps4|ps5)}}
+// XFAIL: target={{.*-windows-msvc.*}}, target={{.*-(ps4|ps5)}}
 
 // For some reason, this fails with a core dump on AIX. This needs to be 
investigated.
 // UNSUPPORTED: target={{.*}}-aix{{.*}}
Index: clang/test/C/drs/dr1xx.c
===
--- clang/test/C/drs/dr1xx.c
+++ clang/test/C/drs/dr1xx.c
@@ -235,7 +235,7 @@
 * type at this point.
 */
 Val = sizeof(enum E)
-#ifndef _WIN32
+#if !defined(_WIN32) || defined(__MINGW32__)
 /* expected-error@-2 {{invalid application of 'sizeof' to an incomplete 
type 'enum E'}} */
 /* expected-note@-12 {{definition of 'enum E' is not complete until the 
closing '}'}} */
 #endif


Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -5,11 +5,11 @@
 // Should not crash.
 template 
 class __attribute__((trivial_abi)) a { a(a &&); };
-#ifdef _WIN64
-// On Windows, to be trivial-for-calls, an object must be trivially copyable.
+#if defined(_WIN64) && !defined(__MINGW32__)
+// On Windows/MSVC, to be trivial-for-calls, an object must be trivially copyable.
 // (And it is only trivially relocatable, currently, if it is trivial for calls.)
 // In this case, it is suppressed by an explicitly defined move constructor.
-// Similar concerns apply to later tests that have #ifdef _WIN64.
+// Similar concerns apply to later tests that have #if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(a), "");
 #else
 static_assert(__is_trivially_relocatable(a), "");
@@ -137,7 +137,7 @@
   CopyDeleted(const CopyDeleted &) = delete;
   CopyDeleted(CopyDeleted &&) = default;
 };
-#ifdef _WIN64
+#if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(CopyDeleted), "");
 #else
 static_assert(__is_trivially_relocatable(CopyDeleted), "");
@@ -163,7 +163,7 @@
 struct __attribute__((trivial_abi)) S20 {
   int & // a member of rvalue reference type deletes the copy constructor.
 };
-#ifdef _WIN64
+#if defined(_WIN64) && !defined(__MINGW32__)
 static_assert(!__is_trivially_relocatable(S20), "");
 #else
 

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D149997#4338304 , @rnk wrote:

> I think `_MSC_EXTENSIONS` will work, but it's kind of gross. `_MSC_VER` is 
> controlled by `-fms-compatibility-verson=`, which I think is off by default 
> or unset at the cc1 level:
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/OSTargets.cpp#L206
> `-fms-extensions` is on by default at cc1, I think.

Ah, I see, thanks!

Since `_MSC_VER` is the most canonical way of checking for an MSVC-like 
environment anyway, do you think it'd make sense to define it to a dummy value 
like 1, if the target is an MSVC triple but no `-fms-compatibility-version=` is 
provided?

Using `_MSC_EXTENSIONS` instead of `_MSC_VER` in these tests isn't very pretty, 
but I guess it could be passable still? (I'm not setting out to improve all the 
world here right now, I'm just trying to get the tests passing in a mingw 
environment.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149997

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

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a subscriber: rnk.
mstorsjo added a comment.

So, the reason why this failed, is that when invoked as `%clang_cc1` in a 
MSVC/clang-cl style build, `_MSC_VER` isn't predefined, while `_WIN32` is. When 
invoked via the Clang driver instead of directly going at `-cc1`, `_MSC_VER` 
does get defined.

@rnk @hans @thakis - who know the intricacies of the MSVC target - do you 
happen to know why that is? How do I distinguish between MSVC-style behaviour 
and other cases (in particular, mingw) in a `%clang_cc1` test? Currently it 
uses `#idef _WIN32` but that's incorrect for mingw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149997

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

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D149997#4331937 , @thakis wrote:

> This broke check-clang on windows: http://45.33.8.238/win/78359/step_7.txt
>
> (The Driver/split-debug.c failure is something else and since fixed, but the 
> other two tests are due to this change.)
>
> Please take a look and revert for now if it takes a while to fix.

Sorry about this, and thanks for reverting. I’ll have a closer look at the 
issue later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149997

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

[PATCH] D149999: [clang-tidy] [test] Narrow down a special case to MSVC mode

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb80febdf43d3: [clang-tidy] [test] Narrow down a special case 
to MSVC mode (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D14

Files:
  
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
@@ -9,7 +9,7 @@
 typedef unsigned short  uint16_t;   // NOLINT
 typedef unsigned long   uint32_t;   // NOLINT
 typedef unsigned long long  uint64_t;   // NOLINT
-#ifndef _WIN32
+#ifndef _MSC_VER
 typedef unsigned long long  size_t; // NOLINT
 #endif
 typedef longintptr_t;   // NOLINT


Index: clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
+++ clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
@@ -9,7 +9,7 @@
 typedef unsigned short  uint16_t;   // NOLINT
 typedef unsigned long   uint32_t;   // NOLINT
 typedef unsigned long long  uint64_t;   // NOLINT
-#ifndef _WIN32
+#ifndef _MSC_VER
 typedef unsigned long long  size_t; // NOLINT
 #endif
 typedef longintptr_t;   // NOLINT
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

[Martin Storsjö 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 rG7f037e5645bd: [clang] [test] Narrow down MSVC specific 
behaviours from any windows to only… (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149997

Files:
  clang/test/C/drs/dr1xx.c
  clang/test/Driver/experimental-library-flag.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp


Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -5,11 +5,11 @@
 // Should not crash.
 template 
 class __attribute__((trivial_abi)) a { a(a &&); };
-#ifdef _WIN64
-// On Windows, to be trivial-for-calls, an object must be trivially copyable.
+#if defined(_WIN64) && defined(_MSC_VER)
+// On Windows/MSVC, to be trivial-for-calls, an object must be trivially 
copyable.
 // (And it is only trivially relocatable, currently, if it is trivial for 
calls.)
 // In this case, it is suppressed by an explicitly defined move constructor.
-// Similar concerns apply to later tests that have #ifdef _WIN64.
+// Similar concerns apply to later tests that have #if defined(_WIN64) && 
defined(_MSC_VER).
 static_assert(!__is_trivially_relocatable(a), "");
 #else
 static_assert(__is_trivially_relocatable(a), "");
@@ -137,7 +137,7 @@
   CopyDeleted(const CopyDeleted &) = delete;
   CopyDeleted(CopyDeleted &&) = default;
 };
-#ifdef _WIN64
+#if defined(_WIN64) && defined(_MSC_VER)
 static_assert(!__is_trivially_relocatable(CopyDeleted), "");
 #else
 static_assert(__is_trivially_relocatable(CopyDeleted), "");
@@ -163,7 +163,7 @@
 struct __attribute__((trivial_abi)) S20 {
   int & // a member of rvalue reference type deletes the copy constructor.
 };
-#ifdef _WIN64
+#if defined(_WIN64) && defined(_MSC_VER)
 static_assert(!__is_trivially_relocatable(S20), "");
 #else
 static_assert(__is_trivially_relocatable(S20), "");
Index: clang/test/Driver/experimental-library-flag.cpp
===
--- clang/test/Driver/experimental-library-flag.cpp
+++ clang/test/Driver/experimental-library-flag.cpp
@@ -1,6 +1,6 @@
 // On some platforms, -stdlib=libc++ is currently ignored, so 
-lc++experimental is not added.
 // Once -stdlib=libc++ works on those, this XFAIL can be removed.
-// XFAIL: target={{.*-windows.*}}, target={{.*-(ps4|ps5)}}
+// XFAIL: target={{.*-windows-msvc.*}}, target={{.*-(ps4|ps5)}}
 
 // For some reason, this fails with a core dump on AIX. This needs to be 
investigated.
 // UNSUPPORTED: target={{.*}}-aix{{.*}}
Index: clang/test/C/drs/dr1xx.c
===
--- clang/test/C/drs/dr1xx.c
+++ clang/test/C/drs/dr1xx.c
@@ -235,7 +235,7 @@
 * type at this point.
 */
 Val = sizeof(enum E)
-#ifndef _WIN32
+#ifndef _MSC_VER
 /* expected-error@-2 {{invalid application of 'sizeof' to an incomplete 
type 'enum E'}} */
 /* expected-note@-12 {{definition of 'enum E' is not complete until the 
closing '}'}} */
 #endif


Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -5,11 +5,11 @@
 // Should not crash.
 template 
 class __attribute__((trivial_abi)) a { a(a &&); };
-#ifdef _WIN64
-// On Windows, to be trivial-for-calls, an object must be trivially copyable.
+#if defined(_WIN64) && defined(_MSC_VER)
+// On Windows/MSVC, to be trivial-for-calls, an object must be trivially copyable.
 // (And it is only trivially relocatable, currently, if it is trivial for calls.)
 // In this case, it is suppressed by an explicitly defined move constructor.
-// Similar concerns apply to later tests that have #ifdef _WIN64.
+// Similar concerns apply to later tests that have #if defined(_WIN64) && defined(_MSC_VER).
 static_assert(!__is_trivially_relocatable(a), "");
 #else
 static_assert(__is_trivially_relocatable(a), "");
@@ -137,7 +137,7 @@
   CopyDeleted(const CopyDeleted &) = delete;
   CopyDeleted(CopyDeleted &&) = default;
 };
-#ifdef _WIN64
+#if defined(_WIN64) && defined(_MSC_VER)
 static_assert(!__is_trivially_relocatable(CopyDeleted), "");
 #else
 static_assert(__is_trivially_relocatable(CopyDeleted), "");
@@ -163,7 +163,7 @@
 struct __attribute__((trivial_abi)) S20 {
   int & // a member of rvalue reference type deletes the copy constructor.
 };
-#ifdef _WIN64
+#if defined(_WIN64) && defined(_MSC_VER)
 static_assert(!__is_trivially_relocatable(S20), "");
 #else
 static_assert(__is_trivially_relocatable(S20), "");
Index: clang/test/Driver/experimental-library-flag.cpp
===
--- 

[PATCH] D149999: [clang-tidy] [test] Narrow down a special case to MSVC mode

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: alvinhochun, aaron.ballman, hans.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: clang-tools-extra.

For MinGW targets, size_t isn't a compiler defined type, just like
for unix targets.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D14

Files:
  
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
@@ -9,7 +9,7 @@
 typedef unsigned short  uint16_t;   // NOLINT
 typedef unsigned long   uint32_t;   // NOLINT
 typedef unsigned long long  uint64_t;   // NOLINT
-#ifndef _WIN32
+#ifndef _MSC_VER
 typedef unsigned long long  size_t; // NOLINT
 #endif
 typedef longintptr_t;   // NOLINT


Index: clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
+++ clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-standard-types.h
@@ -9,7 +9,7 @@
 typedef unsigned short  uint16_t;   // NOLINT
 typedef unsigned long   uint32_t;   // NOLINT
 typedef unsigned long long  uint64_t;   // NOLINT
-#ifndef _WIN32
+#ifndef _MSC_VER
 typedef unsigned long long  size_t; // NOLINT
 #endif
 typedef longintptr_t;   // NOLINT
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: aaron.ballman, hans.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: clang.

This fixes running tests with a toolchain that defaults to a MinGW
target.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149997

Files:
  clang/test/C/drs/dr1xx.c
  clang/test/Driver/experimental-library-flag.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp


Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -5,11 +5,11 @@
 // Should not crash.
 template 
 class __attribute__((trivial_abi)) a { a(a &&); };
-#ifdef _WIN64
-// On Windows, to be trivial-for-calls, an object must be trivially copyable.
+#if defined(_WIN64) && defined(_MSC_VER)
+// On Windows/MSVC, to be trivial-for-calls, an object must be trivially 
copyable.
 // (And it is only trivially relocatable, currently, if it is trivial for 
calls.)
 // In this case, it is suppressed by an explicitly defined move constructor.
-// Similar concerns apply to later tests that have #ifdef _WIN64.
+// Similar concerns apply to later tests that have #if defined(_WIN64) && 
defined(_MSC_VER).
 static_assert(!__is_trivially_relocatable(a), "");
 #else
 static_assert(__is_trivially_relocatable(a), "");
@@ -137,7 +137,7 @@
   CopyDeleted(const CopyDeleted &) = delete;
   CopyDeleted(CopyDeleted &&) = default;
 };
-#ifdef _WIN64
+#if defined(_WIN64) && defined(_MSC_VER)
 static_assert(!__is_trivially_relocatable(CopyDeleted), "");
 #else
 static_assert(__is_trivially_relocatable(CopyDeleted), "");
@@ -163,7 +163,7 @@
 struct __attribute__((trivial_abi)) S20 {
   int & // a member of rvalue reference type deletes the copy constructor.
 };
-#ifdef _WIN64
+#if defined(_WIN64) && defined(_MSC_VER)
 static_assert(!__is_trivially_relocatable(S20), "");
 #else
 static_assert(__is_trivially_relocatable(S20), "");
Index: clang/test/Driver/experimental-library-flag.cpp
===
--- clang/test/Driver/experimental-library-flag.cpp
+++ clang/test/Driver/experimental-library-flag.cpp
@@ -1,6 +1,6 @@
 // On some platforms, -stdlib=libc++ is currently ignored, so 
-lc++experimental is not added.
 // Once -stdlib=libc++ works on those, this XFAIL can be removed.
-// XFAIL: target={{.*-windows.*}}, target={{.*-(ps4|ps5)}}
+// XFAIL: target={{.*-windows-msvc.*}}, target={{.*-(ps4|ps5)}}
 
 // For some reason, this fails with a core dump on AIX. This needs to be 
investigated.
 // UNSUPPORTED: target={{.*}}-aix{{.*}}
Index: clang/test/C/drs/dr1xx.c
===
--- clang/test/C/drs/dr1xx.c
+++ clang/test/C/drs/dr1xx.c
@@ -235,7 +235,7 @@
 * type at this point.
 */
 Val = sizeof(enum E)
-#ifndef _WIN32
+#ifndef _MSC_VER
 /* expected-error@-2 {{invalid application of 'sizeof' to an incomplete 
type 'enum E'}} */
 /* expected-note@-12 {{definition of 'enum E' is not complete until the 
closing '}'}} */
 #endif


Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -5,11 +5,11 @@
 // Should not crash.
 template 
 class __attribute__((trivial_abi)) a { a(a &&); };
-#ifdef _WIN64
-// On Windows, to be trivial-for-calls, an object must be trivially copyable.
+#if defined(_WIN64) && defined(_MSC_VER)
+// On Windows/MSVC, to be trivial-for-calls, an object must be trivially copyable.
 // (And it is only trivially relocatable, currently, if it is trivial for calls.)
 // In this case, it is suppressed by an explicitly defined move constructor.
-// Similar concerns apply to later tests that have #ifdef _WIN64.
+// Similar concerns apply to later tests that have #if defined(_WIN64) && defined(_MSC_VER).
 static_assert(!__is_trivially_relocatable(a), "");
 #else
 static_assert(__is_trivially_relocatable(a), "");
@@ -137,7 +137,7 @@
   CopyDeleted(const CopyDeleted &) = delete;
   CopyDeleted(CopyDeleted &&) = default;
 };
-#ifdef _WIN64
+#if defined(_WIN64) && defined(_MSC_VER)
 static_assert(!__is_trivially_relocatable(CopyDeleted), "");
 #else
 static_assert(__is_trivially_relocatable(CopyDeleted), "");
@@ -163,7 +163,7 @@
 struct __attribute__((trivial_abi)) S20 {
   int & // a member of rvalue reference type deletes the copy constructor.
 };
-#ifdef _WIN64
+#if defined(_WIN64) && defined(_MSC_VER)
 static_assert(!__is_trivially_relocatable(S20), "");
 #else
 static_assert(__is_trivially_relocatable(S20), "");
Index: clang/test/Driver/experimental-library-flag.cpp
===
--- clang/test/Driver/experimental-library-flag.cpp
+++ clang/test/Driver/experimental-library-flag.cpp
@@ 

[PATCH] D148944: [clang][Driver] Fix crash with unsupported architectures in MinGW and CrossWindows.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/test/Driver/unsupported-target-arch.c:33
+// RUN: FileCheck --input-file=%t.err --check-prefix=CHECK-NOARCH-CROSSWINDOWS 
%s
+// CHECK-NOARCH-CROSSWINDOWS: error: unknown target triple 
'noarch-unknown-windows-itanium', please use -triple or -arch

MaskRay wrote:
> mstorsjo wrote:
> > Separate side note; the suggested option `-triple` isn't really a clang 
> > driver level option, and `-arch` is only relevant for darwin targets 
> > (AFAIK) - so maybe the error message text should be revised?
> The part ", please use -triple or -arch" is almost always wrong.
> 
> For cc1, the error is due to a specified but unknown -triple, we should not 
> suggest specifying -triple.
> For driver, -triple and -arch are not driver options.
> 
> I just removed the hint from the diagnostic.
`-arch` is a driver option, for darwin targets though. But I agree that it's 
probably best to leave it out since it's more misleading than helping here (and 
`-triple` is indeed not a relevant option here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148944

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

[PATCH] D148944: [clang][Driver] Fix crash with unsupported architectures in MinGW and CrossWindows.

[Martin Storsjö via Phabricator via cfe-commits]

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

LGTM. If you don’t have commit access, please say your preferred git author 
name for the commit, i.e. `Real Name `.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148944

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

[PATCH] D146490: [Support] On Windows, ensure that UniqueID is really stable

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D146490#4307269 , @falhumai96 
wrote:

> I would like to follow up on this issue whether or not there has been an 
> update on it.

There's no update currently; I think there's some amount of consensus that this 
approach, while fixing the issue, would cause too much other collateral issues.

I'm considering making a patch that switches to hashing the canonicalized path, 
for the cases where we know the path isn't a local path; it's not an entirely 
correct solution, but a rough approximation for when the file IDs are stable or 
not. I just haven't had time to try doing that yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146490

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

[PATCH] D148944: [clang][Driver] Fix crash with unsupported architectures in MinGW and CrossWindows.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

I think the code changes make sense; these are trivial to hit by a user, so 
they shouldn't be `llvm_unreachable`.




Comment at: clang/test/Driver/unsupported-target-arch.c:33
+// RUN: FileCheck --input-file=%t.err --check-prefix=CHECK-NOARCH-CROSSWINDOWS 
%s
+// CHECK-NOARCH-CROSSWINDOWS: error: unknown target triple 
'noarch-unknown-windows-itanium', please use -triple or -arch

Separate side note; the suggested option `-triple` isn't really a clang driver 
level option, and `-arch` is only relevant for darwin targets (AFAIK) - so 
maybe the error message text should be revised?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148944

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

[PATCH] D149119: [CMake] Use llvm-nm to extract symbols for staged LTO builds on Windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D149119#4298024 , @phosek wrote:

> Supporting only a single tool and simplifying the script would be my 
> preference as well. I see that the script already supports `llvm-readobj`, do 
> we need the `llvm-nm` support in that case?

I remember seeing it mentioned somewhere (I don't immediately see where right 
now though), `llvm-readobj` only handles regular object files, while `llvm-nm` 
handles bitcode too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149119

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

[PATCH] D146987: [Assignment Tracking] Enable by default

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D146987#4286797 , @jmorse wrote:

> /me grumbles about all the world being a VAX,
>
> @mstorsjo I can't replicate the crash, but can replicate the valgrind 
> jump-on-uninitialized-value with a small reproducer [0] that doesn't feature 
> any debug-info

Ok, it's possible that bit was a red herring here. It didn't show up in a build 
of Clang instrumented with asan either.

> Could you confirm it's definitely assignment-tracking at fault by using 
> `-Xclang -fexperimental-assignment-tracking=forced` to enable and `-Xclang 
> -fexperimental-assignment-tracking=disabled` to disable, which should control 
> the behaviour if it's AT at fault.

The `-Xclang -fexperimental-assignment-tracking=disabled` flag does make the 
assert that shows up when built with Xcode's clang go away at least. It doesn't 
affect the valgrind failure, so that's indeed unrelated.

So there's something in Clang/LLVM which behaves differently, to the point of 
triggering a failed assert, when built with Xcode's Clang (reproed with two 
different Xcode versions) but not on Linux (with GCC or Clang, at least with a 
possibly older Clang).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146987

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

[PATCH] D146987: [Assignment Tracking] Enable by default

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

I'm also running into issues due to this commit. Unfortunately, the issues 
don't seem to be entirely deterministic. They can be reproduced with 
https://martin.st/temp/python-preproc.c:

  $ clang -target i686-w64-mingw32 -c -g -O3 python-preproc.c
  Assertion failed: (!(Elmt.getFragmentOrDefault() == 
Next.getFragmentOrDefault())), function operator(), file 
AssignmentTrackingAnalysis.cpp, line 2020.

I'm seeing this on macOS with LLVM built with Xcode's clang. On Linux, built 
with either GCC or Clang, I don't run into this issue.

When running the seemingly working compiler build on Linux in valgrind, I'm 
hitting use of uninitialized data:

  ==3054473== Conditional jump or move depends on uninitialised value(s)
  ==3054473==at 0x4900F0A: 
llvm::ConstantExpr::getGetElementPtr(llvm::Type*, llvm::Constant*, 
llvm::ArrayRef, bool, std::optional, llvm::Type*) 
(in /home/martin/code/llvm-project-bisect/llvm/build/bin/clang-17)
  ==3054473==by 0x48C865F: foldGEPOfGEP(llvm::GEPOperator*, llvm::Type*, 
bool, llvm::ArrayRef) (in 
/home/martin/code/llvm-project-bisect/llvm/build/bin/clang-17)
  ==3054473==by 0x48D1285: llvm::ConstantFoldGetElementPtr(llvm::Type*, 
llvm::Constant*, bool, std::optional, 
llvm::ArrayRef) (in 
/home/martin/code/llvm-project-bisect/llvm/build/bin/clang-17)
  ==3054473==by 0x4900CEF: 
llvm::ConstantExpr::getGetElementPtr(llvm::Type*, llvm::Constant*, 
llvm::ArrayRef, bool, std::optional, llvm::Type*) 
(in /home/martin/code/llvm-project-bisect/llvm/build/bin/clang-17)
  ==3054473==by 0x4025C21: simplifyGEPInst(llvm::Type*, llvm::Value*, 
llvm::ArrayRef, bool, llvm::SimplifyQuery const&, unsigned int) 
[clone .constprop.0] (in 
/home/martin/code/llvm-project-bisect/llvm/build/bin/clang-17)
  ==3054473==by 0x4036E5F: 
simplifyInstructionWithOperands(llvm::Instruction*, 
llvm::ArrayRef, llvm::SimplifyQuery const&, unsigned int) [clone 
.constprop.0] (in /home/martin/code/llvm-project-bisect/llvm/build/bin/clang-17)
  ==3054473==by 0x4037423: llvm::simplifyInstruction(llvm::Instruction*, 
llvm::SimplifyQuery const&) (in 
/home/martin/code/llvm-project-bisect/llvm/build/bin/clang-17)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146987

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

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

FYI, I'm seeing occasional spurious build breakage from this change.

The issue is that even if we've got 
`add_dependencies(clangAnalysisFlowSensitive 
clangAnalysisFlowSensitiveResources)`, it looks like 
`clangAnalysisFlowSensitiveResources` only is considered a dependency to the 
whole `clangAnalysisFlowSensitive` library target, but the individual 
`HTMLLogger.cpp.o` object file doesn't have a dependency on the generated 
`HTMLLogger.inc` file. So when building, it may start building 
`HTMLLogger.cpp.o` before `HTMLLogger.inc` has been generated.

To reproduce it:

  $ cd llvm-project/llvm
  $ mkdir build
  $ cd build
  $ cmake -G Ninja .. -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang"
  $ ninja 
tools/clang/lib/Analysis/FlowSensitive/CMakeFiles/obj.clangAnalysisFlowSensitive.dir/HTMLLogger.cpp.o
  ../tools/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:72:10: fatal error: 
HTMLLogger.inc: No such file or directory
 72 | #include "HTMLLogger.inc"
|  ^~~~

If you've built this successfully once, then ninja also has picked up the 
dependency between `HTMLLogger.cpp.o` and `HTMLLogger.inc`, but it's possible 
to reproduce this in a clean build dir with the commands above.

Looking at the generated `build.ninja`, we've got this:

  build cmake_object_order_depends_target_obj.clangAnalysisFlowSensitive: phony 
|| tools/clang/clang-tablegen-targets
  
  build cmake_object_order_depends_target_clangAnalysisFlowSensitive: phony || 
cmake_object_order_depends_target_LLVMAggressiveInstCombine 
cmake_object_order_depends_target_LLVMAnalysis 
cmake_object_order_depends_target_LLVMAsmParser 
cmake_object_order_depends_target_LLVMBinaryFormat 
cmake_object_order_depends_target_LLVMBitReader 
cmake_object_order_depends_target_LLVMBitstreamReader 
cmake_object_order_depends_target_LLVMCore 
cmake_object_order_depends_target_LLVMDebugInfoCodeView 
cmake_object_order_depends_target_LLVMDebugInfoDWARF 
cmake_object_order_depends_target_LLVMDebugInfoMSF 
cmake_object_order_depends_target_LLVMDebugInfoPDB 
cmake_object_order_depends_target_LLVMDemangle 
cmake_object_order_depends_target_LLVMFrontendOpenMP 
cmake_object_order_depends_target_LLVMIRReader 
cmake_object_order_depends_target_LLVMInstCombine 
cmake_object_order_depends_target_LLVMMC 
cmake_object_order_depends_target_LLVMMCParser 
cmake_object_order_depends_target_LLVMObject 
cmake_object_order_depends_target_LLVMProfileData 
cmake_object_order_depends_target_LLVMRemarks 
cmake_object_order_depends_target_LLVMScalarOpts 
cmake_object_order_depends_target_LLVMSupport 
cmake_object_order_depends_target_LLVMSymbolize 
cmake_object_order_depends_target_LLVMTargetParser 
cmake_object_order_depends_target_LLVMTextAPI 
cmake_object_order_depends_target_LLVMTransformUtils 
cmake_object_order_depends_target_clangAST 
cmake_object_order_depends_target_clangASTMatchers 
cmake_object_order_depends_target_clangAnalysis 
cmake_object_order_depends_target_clangBasic 
cmake_object_order_depends_target_clangLex 
cmake_object_order_depends_target_obj.clangAnalysisFlowSensitive 
tools/clang/clang-tablegen-targets 
tools/clang/lib/Analysis/FlowSensitive/clangAnalysisFlowSensitiveResources
  
  build 
tools/clang/lib/Analysis/FlowSensitive/CMakeFiles/obj.clangAnalysisFlowSensitive.dir/HTMLLogger.cpp.o:
 CXX_COMPILER__obj.2eclangAnalysisFlowSensitive_Release 
../tools/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp || 
cmake_object_order_depends_target_obj.clangAnalysisFlowSensitive

`HTMLLogger.cpp.o` depends on 
`cmake_object_order_depends_target_obj.clangAnalysisFlowSensitive`, but that 
one doesn't depend on `clangAnalysisFlowSensitiveResources`. Only 
`cmake_object_order_depends_target_clangAnalysisFlowSensitive` (note the 
missing `obj.` in the middle) depends on it.

Looking at the similarly handled `HTMLForestResources.inc` in `clang-pseudo`, 
that one does work correctly. That one is set up with `add_clang_tool`, where 
there's no intermediate object library between the target itself and the object 
files as for `add_clang_library`.

Not sure what the right CMake way to handle it is - should the dependency be 
added for the intermediate object library? (Looking in the cmake files, it 
doesn't seem like we always generate object libraries.) Or should we add an 
explicit dependency for the `HTMLLogger.cpp` source file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591

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

[PATCH] D148751: [CMake] Add llvm-lib to Clang bootstrap dependency for LTO builds on Windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/CMakeLists.txt:616
 -DDYLD_LIBRARY_PATH=${LLVM_LIBRARY_OUTPUT_INTDIR})
-elseif(NOT WIN32)
+elseif(WIN32)
+  add_dependencies(clang-bootstrap-deps llvm-lib)

I think I’d prefer to have the condition be `MSVC`, not `WIN32`, as we don’t 
want to use `llvm-lib` in the case of mingw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148751

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

[PATCH] D146908: [clang][MinGW] Add asan DLL lib before other libs and objects

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/test/Driver/mingw-sanitizers.c:2
+// RUN: touch %t.a
+// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address -lcomponent 
%/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-I686 -DINPUT=%/t.a %s
+// RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=address 
-lcomponent %/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-X86_64 
-DINPUT=%/t.a %s

alvinhochun wrote:
> mstorsjo wrote:
> > You're using `%/t.a` here while the file you touch is `%t.a` - I don't 
> > think I have seen `%/t` anywhere here before. I do see that lit seems to 
> > replace it though... Is this an intentional thing (what's the difference to 
> > `%t` though?) or is it a consistently copied typo?
> Ah yes, that is intentional. It's documented on 
> https://llvm.org/docs/CommandGuide/lit.html#substitutions to be "%t but \ is 
> replaced by /". The reason for this is that, the previous Windows pre-merge 
> check failed because when `clang -###` prints the command line it quotes and 
> backslash-escape the arguments., FileCheck cannot match the INPUT path with 
> double backslashes. Using forward slashes avoids this issue.
Oh, I see, thanks - TIL! That makes sense. I wonder why a grep for that didn't 
get almost any hits in the clang/test/Driver directory though - but maybe it's 
used more elsewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146908

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

[PATCH] D146908: [clang][MinGW] Add asan DLL lib before other libs and objects

[Martin Storsjö via Phabricator via cfe-commits]

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

LGTM overall.

I wondered if the modified test runs correctly on Windows (any test that 
touches paths is prone to break) but I see that it seems to have run 
successfully on both Windows and Linux in the premerge CI, so that's at least 
good. One minor question for the test.




Comment at: clang/test/Driver/mingw-sanitizers.c:2
+// RUN: touch %t.a
+// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address -lcomponent 
%/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-I686 -DINPUT=%/t.a %s
+// RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=address 
-lcomponent %/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-X86_64 
-DINPUT=%/t.a %s

You're using `%/t.a` here while the file you touch is `%t.a` - I don't think I 
have seen `%/t` anywhere here before. I do see that lit seems to replace it 
though... Is this an intentional thing (what's the difference to `%t` though?) 
or is it a consistently copied typo?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146908

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

[PATCH] D146908: [clang][MinGW] Add asan link flags before other libs and objects

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/test/Driver/mingw-sanitizers.c:1
-// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address 2>&1 | 
FileCheck --check-prefix=ASAN-I686 %s
-// ASAN-I686: "{{.*}}libclang_rt.asan_dynamic-i386.dll.a"
-// ASAN-I686: "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a"
-// ASAN-I686: "--require-defined" "___asan_seh_interceptor"
-// ASAN-I686: "--whole-archive" 
"{{.*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a" "--no-whole-archive"
-
-// RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=address 2>&1 | 
FileCheck --check-prefix=ASAN-X86_64 %s
-// ASAN-X86_64: "{{.*}}libclang_rt.asan_dynamic-x86_64.dll.a"
+// RUN: echo -n > %t.a
+// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address -lcomponent 
%/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-I686 -DINPUT=%/t.a %s

alvinhochun wrote:
> mstorsjo wrote:
> > alvinhochun wrote:
> > > mstorsjo wrote:
> > > > Hm, what does this do - some sort of more portable version of just 
> > > > touching a file to create it?
> > > Yeah that's just creating the file (though it would also overwrite the 
> > > existing content). I believe lit has `echo` built-in but `touch` may not 
> > > be available on Windows so this seems like a good idea to me.
> > Ah, I see. FWIW, the lit tests in general do assume a handful of unix 
> > utilities - we seem to have unconditional uses of `touch` in various tests. 
> > The lit tests check if these tools are available in path, and if they 
> > aren't, it tries to locate an installation of Git for Windows (which 
> > contains what's needed) and add that to the path internally: 
> > https://github.com/llvm/llvm-project/blob/llvmorg-17-init/llvm/utils/lit/lit/llvm/config.py#L29-L35
> I suppose that works. Would you prefer to use touch here too?
I don't have a strong opinion, but with touch I know what is intended, while 
this stopped my reading, wondering what the flag to `echo` does and if this is 
meant to have any specific function other than creating the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146908

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

[PATCH] D146908: [clang][MinGW] Add asan link flags before other libs and objects

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/test/Driver/mingw-sanitizers.c:1
-// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address 2>&1 | 
FileCheck --check-prefix=ASAN-I686 %s
-// ASAN-I686: "{{.*}}libclang_rt.asan_dynamic-i386.dll.a"
-// ASAN-I686: "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a"
-// ASAN-I686: "--require-defined" "___asan_seh_interceptor"
-// ASAN-I686: "--whole-archive" 
"{{.*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a" "--no-whole-archive"
-
-// RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=address 2>&1 | 
FileCheck --check-prefix=ASAN-X86_64 %s
-// ASAN-X86_64: "{{.*}}libclang_rt.asan_dynamic-x86_64.dll.a"
+// RUN: echo -n > %t.a
+// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address -lcomponent 
%/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-I686 -DINPUT=%/t.a %s

alvinhochun wrote:
> mstorsjo wrote:
> > Hm, what does this do - some sort of more portable version of just touching 
> > a file to create it?
> Yeah that's just creating the file (though it would also overwrite the 
> existing content). I believe lit has `echo` built-in but `touch` may not be 
> available on Windows so this seems like a good idea to me.
Ah, I see. FWIW, the lit tests in general do assume a handful of unix utilities 
- we seem to have unconditional uses of `touch` in various tests. The lit tests 
check if these tools are available in path, and if they aren't, it tries to 
locate an installation of Git for Windows (which contains what's needed) and 
add that to the path internally: 
https://github.com/llvm/llvm-project/blob/llvmorg-17-init/llvm/utils/lit/lit/llvm/config.py#L29-L35


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146908

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

[PATCH] D146908: [clang][MinGW] Add asan link flags before other libs and objects

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

I tested this, and this does fix the repro from the linked issue.

I wonder if we do want to move all of these before the app itself, or if it's 
enough to just move the reference to `asan_dynamic-.dll.a` earlier, and 
keep the rest as it was? Especially the whole-archive bit ends up linking a 
bunch of things statically, ordered way before the app itself. On one hand, I 
wonder if it would have subtle effects that we don't notice until way later, 
and we should only change as little as we need (moving the import library 
reference earlier), or if there are other potential benefits to moving the 
wholearchive-linked bits earlier too? (I'm wondering about things like 
constructor execution orders and such.)




Comment at: clang/test/Driver/mingw-sanitizers.c:1
-// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address 2>&1 | 
FileCheck --check-prefix=ASAN-I686 %s
-// ASAN-I686: "{{.*}}libclang_rt.asan_dynamic-i386.dll.a"
-// ASAN-I686: "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a"
-// ASAN-I686: "--require-defined" "___asan_seh_interceptor"
-// ASAN-I686: "--whole-archive" 
"{{.*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a" "--no-whole-archive"
-
-// RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=address 2>&1 | 
FileCheck --check-prefix=ASAN-X86_64 %s
-// ASAN-X86_64: "{{.*}}libclang_rt.asan_dynamic-x86_64.dll.a"
+// RUN: echo -n > %t.a
+// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address -lcomponent 
%/t.a 2>&1 | FileCheck --check-prefixes=ASAN-ALL,ASAN-I686 -DINPUT=%/t.a %s

Hm, what does this do - some sort of more portable version of just touching a 
file to create it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146908

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D146764#4219278 , @hans wrote:

> +mstorsjo is this okay for mingw mode too?

I believe the current form of the patch is fine - i.e. disabled by default in 
mingw mode, but enabled if the extra MS language extensions are enabled. (I 
didn't check myself how those options map to `LangOpts.MicrosoftExt` but I 
presume it works as I described above.)




Comment at: clang/include/clang/Testing/TestClangConfig.h:61
 
-  bool hasDelayedTemplateParsing() const {
-return Target == "x86_64-pc-win32-msvc";
-  }
+  bool isWin32() const { return Target == "x86_64-pc-win32-msvc"; }
+

I'm not entirely sure of the use context of this function (only specific 
tests?), but mingw targets with a differen triple are win32 too. But then they 
could also be using an entirely different architecture than x86_64, so I guess 
this might be ok too if we're ready to tweak it when a need arises?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

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

[PATCH] D146490: [Support] On Windows, ensure that UniqueID is really stable

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D146490#4209495 , @aganea wrote:

> Fair enough. There are several choices forward: either we mark the issue as 
> "Will Not Fix" or I can try only scoping this patch to only keep the handle 
> for network drives/paths. Any other suggestions?

I also kinda share the sentiment that keeping handles open might be problematic 
- the cure is worse than the disease. I wouldn't quite start throwing in the 
towel quite yet though...

While the docs say that the ID only is stable as long as the handles are open 
(except for NTFS where it's an implementation detail), it's also stable enough 
for FAT (as long as nobody is defragmenting the drive while the process is 
open); at least stable enough for compiler/linker use, but possibly not for 
long running processes like clangd.

It might not even be strictly necessary for all network drives; out of 4 tested 
cases, it seemed to behave fine (stable enough at least) with a Linux Samba 
mount and a drive shared from VMWare Fusion, while Remote Desktop folder 
sharing (and allegedly VirtualBox although I haven't tried that) exhibit the 
issue. But I don't know of any immediate way of identifying the problematic 
cases either...

Another potential band-aid solution is to simply hash the pathnames for such 
drives. We won't get the desired effect of correctly identifying the same 
individual file via different pathnames (hardlinks/symlinks), but we would at 
least have a stable representation of a file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146490

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

[PATCH] D146165: docs: add some documentation on Windows SDK search

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Looks reasonable I guess - but I think it would be good to mention the env 
variables `INCLUDE` and `LIB` too, for alternative ways of finding the same 
things - even if it's not strictly the same as what this new section talks 
about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146165

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

[PATCH] D144651: [Serialization] Place command line defines in the correct file

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a reviewer: aaron.ballman.
mstorsjo added a subscriber: aaron.ballman.
mstorsjo added a comment.

Adding @aaron.ballman as fallback Clang reviewer here. While I did touch code 
in the vicinity of this area recently, I'm not familiar enough with the whole 
area to take on reviewing it at the moment.


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

https://reviews.llvm.org/D144651

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

[PATCH] D145007: Driver: introduce GNU spellings to control MSVC paths

[Martin Storsjö via Phabricator via cfe-commits]

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

If others don't have anything to add on this matter, I guess this is fine.

The new option names feel rather wordy and unwieldy (especially compared to the 
clang-cl forms they're aliases for), so if you would want to consider a 
slightly shorter form, that would also be fine with me. But I won't object to 
this form either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145007

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

[PATCH] D145007: Driver: introduce GNU spellings to control MSVC paths

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added reviewers: akhuang, hans, thakis, zahen, pkasting.
mstorsjo added a comment.

Looks reasonable to me, adding a few others for opinions. Should we have some 
tests for this? I guess testing these is somewhat environment reliant though, 
so it's probably not so easy...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145007

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

[PATCH] D142224: [Support] Emulate SIGPIPE handling in raw_fd_ostream write for Windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: llvm/lib/Support/Windows/Signals.inc:834
+  int RetCode = (int)EP->ExceptionRecord->ExceptionCode;
+  if (RetCode == (0xE000 | EX_IOERR))
+return;

erichkeane wrote:
> aganea wrote:
> > erichkeane wrote:
> > > This patch seems to cause a self-build Werror regression.  The mask here 
> > > is large enough to cause the RHS of this to be unsigned, so the 
> > > comparison hits `-Wsign-compare`. See the example here 
> > > https://godbolt.org/z/fa5q889jh
> > Please see rG1d0a5f11c04e6ac4dab578b81d02eabb83b31428
> Wonderful, thanks!  Looks like our build system didn't get to that yet.
Thanks @aganea for the fix! This has been backported to 16.x, so I filed a 
request to backport your warning fix too, see 
https://github.com/llvm/llvm-project/issues/61012.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142224

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

[PATCH] D142297: [Clang][OpenMP] Find the type `omp_allocator_handle_t` from identifier table

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This change broke the `parallel/bug54082.c` testcase in the OpenMP runtime test 
set, when running on Windows (both mingw and MSVC configurations, and happening 
both in i386 and x86_64 builds), see e.g. 
https://github.com/mstorsjo/llvm-mingw/actions/runs/4011290068/jobs/6891673869. 
It's failing with error code `0xC0FD`, which means `STATUS_STACK_OVERFLOW`. 
Is this change missing some aspect that differs between the unix and Windows 
ABIs?

So please don't backport this to the release branch right now. And I'd 
appreciate if you would consider to revert it if there's no immediately obvious 
fix in sight.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142297

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

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: llvm/include/llvm/Support/CodeGen.h:67
+  inline std::optional getLevel(IDType ID) {
+if (ID < 0 || ID > 3)
+  return std::nullopt;

scott.linder wrote:
> barannikov88 wrote:
> > As I can see, clients do not check for nullopt. Either add checks or 
> > replace this check with an assertion and drop std::optional (in this case 
> > `parseLevel` should be updated accordingly).
> > 
> > 
> Good catch! I was working off of the old behavior of `llvm::Optional` and 
> assuming the new `std::optional` was guaranteed to `assert` on dereference as 
> well. I think the right interface is to expose the `optional`, so for the 
> callsites which currently do the equivalent of asserting I will just carry 
> over an explicit assert to the new version.
> 
> I posted to 
> https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716/26
>  to discuss the issue more generally, as at least some of the patches which 
> moved code from `llvm::Optional` to `std::optional` accidentally dropped 
> assertions.
This causes massive amounts of warnings when built with GCC:
```
../include/llvm/Support/CodeGen.h:67:12: warning: comparison is always false 
due to limited range of data type [-Wtype-limits]
   67 | if (ID < 0 || ID > 3)
  | ~~~^~~
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141968

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

[PATCH] D142346: [docs] Add release notes for news in 16.x done by me, or otherwise relating to MinGW targets

[Martin Storsjö 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 rGc3737a652230: [docs] Add release notes for news in 16.x done 
by me, or otherwise relating to… (authored by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D142346?vs=491281=491479#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142346

Files:
  clang/docs/ReleaseNotes.rst
  lld/docs/ReleaseNotes.rst
  llvm/docs/ReleaseNotes.rst
  openmp/docs/ReleaseNotes.rst

Index: openmp/docs/ReleaseNotes.rst
===
--- openmp/docs/ReleaseNotes.rst
+++ openmp/docs/ReleaseNotes.rst
@@ -19,3 +19,8 @@
 
 Non-comprehensive list of changes in this release
 =
+
+* Support for building the OpenMP runtime for Windows on AArch64 and ARM
+  with MinGW based toolchains.
+
+* Made the OpenMP runtime tests run successfully on Windows.
Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -222,6 +222,25 @@
   an affinity mask issue.
   (`D138747 `_)
 
+* When building LLVM and related tools for Windows with Clang in MinGW mode,
+  hidden symbol visiblity is now used to reduce the number of exports in
+  builds with dylibs (``LLVM_BUILD_LLVM_DYLIB`` or ``LLVM_LINK_LLVM_DYLIB``),
+  making such builds more manageable without running into the limit of
+  number of exported symbols.
+
+* AArch64 SEH unwind info generation bugs have been fixed; there were minor
+  cases of mismatches between the generated unwind info and actual
+  prologues/epilogues earlier in some cases.
+
+* AArch64 SEH unwind info is now generated correctly for the AArch64
+  security features BTI (Branch Target Identification) and PAC (Pointer
+  Authentication Code). In particular, using PAC with older versions of LLVM
+  would generate code that would fail to unwind at runtime, if the host
+  actually would use the pointer authentication feature.
+
+* Fixed stack alignment on Windows on AArch64, for stack frames with a
+  large enough allocation that requires stack probing.
+
 Changes to the X86 Backend
 --
 
@@ -308,11 +327,20 @@
 * ``llvm-objdump`` now uses ``--print-imm-hex`` by default, which brings its
   default behavior closer in line with ``objdump``.
 
+* ``llvm-objcopy`` no longer writes corrupt addresses to empty sections if
+  the input file had a nonzero address to an empty section.
+
 Changes to LLDB
 -
 
 * Initial support for debugging Linux LoongArch 64-bit binaries.
 
+* Improvements in COFF symbol handling; previously a DLL (without any other
+  debug info) would only use the DLL's exported symbols, while it now also
+  uses the full list of internal symbols, if available.
+
+* Avoiding duplicate DLLs in the runtime list of loaded modules on Windows.
+
 Changes to Sanitizers
 -
 
@@ -329,6 +357,12 @@
   would now be ``UNSUPPORTED: target=arm{{.*}}`` and ``XFAIL: windows``
   would now be ``XFAIL: target={{.*}}-windows{{.*}}``.
 
+* When cross compiling LLVM (or building with ``LLVM_OPTIMIZED_TABLEGEN``),
+  it is now possible to point the build to prebuilt versions of all the
+  host tools with one CMake variable, ``LLVM_NATIVE_TOOL_DIR``, instead of
+  having to point out each individual tool with variables such as
+  ``LLVM_TABLEGEN``, ``CLANG_TABLEGEN``, ``LLDB_TABLEGEN`` etc.
+
 External Open Source Projects Using LLVM 15
 ===
 
Index: lld/docs/ReleaseNotes.rst
===
--- lld/docs/ReleaseNotes.rst
+++ lld/docs/ReleaseNotes.rst
@@ -57,6 +57,8 @@
 * Improvements to the PCH.OBJ files handling. Now LLD behaves the same as MSVC
   link.exe when merging PCH.OBJ files that don't have the same signature.
   (`D136762 `_)
+* Changed the OrdinalBase for DLLs from 0 to 1, matching the output from
+  both MS link.exe and GNU ld. (`D134140 `_)
 
 MinGW Improvements
 --
@@ -71,6 +73,14 @@
   the load config directory and be filled with the required symbols.
   (`D132808 `_)
 
+* Pick up libraries named ``.lib`` when linked with ``-l``, even
+  if ``-static`` has been specified. This fixes conformance to what
+  GNU ld does. (`D135651 `_)
+
+* Unwinding in Rust code on i386 in MinGW builds has been fixed, by avoiding
+  to leave out the ``rust_eh_personality`` symbol.
+  (`D136879 `_)
+
 MachO Improvements
 --
 
Index: clang/docs/ReleaseNotes.rst