[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd closed this revision.
compnerd added a comment.

SVN r315126


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: lib/Basic/Targets/AArch64.cpp:47-51
+  bool IsNetBSD = getTriple().getOS() == llvm::Triple::NetBSD;
+  bool IsOpenBSD = getTriple().getOS() == llvm::Triple::OpenBSD;
+  if (!getTriple().isOSDarwin() && !IsNetBSD && !IsOpenBSD)
+WCharType = UnsignedInt;
+

rnk wrote:
> I felt like this was clearer the way it was before, and we're already 
> checking for the BSDs above.
Okay, I'll swap it back.



Comment at: lib/Basic/Targets/AArch64.cpp:160-161
 
-  Builder.defineMacro("__ARM_SIZEOF_WCHAR_T", Opts.ShortWChar ? "2" : "4");
+  Builder.defineMacro("__ARM_SIZEOF_WCHAR_T",
+  llvm::utostr(Opts.WCharSize ? Opts.WCharSize : 4));
 

rnk wrote:
> This is correct because we compute macros after we apply the flag override, 
> right?
Correct :-)


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Reid Kleckner via Phabricator via cfe-commits]

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

Looks good with nits




Comment at: lib/Basic/Targets/AArch64.cpp:47-51
+  bool IsNetBSD = getTriple().getOS() == llvm::Triple::NetBSD;
+  bool IsOpenBSD = getTriple().getOS() == llvm::Triple::OpenBSD;
+  if (!getTriple().isOSDarwin() && !IsNetBSD && !IsOpenBSD)
+WCharType = UnsignedInt;
+

I felt like this was clearer the way it was before, and we're already checking 
for the BSDs above.



Comment at: lib/Basic/Targets/AArch64.cpp:160-161
 
-  Builder.defineMacro("__ARM_SIZEOF_WCHAR_T", Opts.ShortWChar ? "2" : "4");
+  Builder.defineMacro("__ARM_SIZEOF_WCHAR_T",
+  llvm::utostr(Opts.WCharSize ? Opts.WCharSize : 4));
 

This is correct because we compute macros after we apply the flag override, 
right?


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 118085.
compnerd added a comment.

Split the defaulting back to all the various targets.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/Basic/LangOptions.def
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets/AArch64.cpp
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/AVR.h
  lib/Basic/Targets/OSTargets.h
  lib/Basic/Targets/X86.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CXX/conv/conv.prom/p2.cpp
  test/CodeGen/arm-metadata.c
  test/CodeGen/pascal-wchar-string.c
  test/CodeGen/string-literal-short-wstring.c
  test/CodeGen/string-literal-unicode-conversion.c
  test/CodeGen/wchar-size.c
  test/Driver/clang_f_opts.c
  test/Headers/wchar_limits.cpp
  test/Index/index-pch.cpp
  test/Lexer/wchar.c
  test/Preprocessor/init.c
  test/Preprocessor/pr19649-unsigned-wchar_t.c
  test/Preprocessor/wchar_t.c
  test/Sema/wchar.c
  test/SemaCXX/short-wchar-sign.cpp

Index: test/SemaCXX/short-wchar-sign.cpp
===
--- test/SemaCXX/short-wchar-sign.cpp
+++ test/SemaCXX/short-wchar-sign.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple i386-mingw32 -fsyntax-only -pedantic -verify %s
-// RUN: %clang_cc1 -fshort-wchar -fsyntax-only -pedantic -verify %s
+// RUN: %clang_cc1 -fwchar-type=short -fno-signed-wchar -fsyntax-only -pedantic -verify %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -pedantic -verify %s
 // expected-no-diagnostics
 
Index: test/Sema/wchar.c
===
--- test/Sema/wchar.c
+++ test/Sema/wchar.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// RUN: %clang_cc1 %s -fsyntax-only -fshort-wchar -verify -DSHORT_WCHAR
+// RUN: %clang_cc1 %s -fsyntax-only -fwchar-type=short -fno-signed-wchar -verify -DSHORT_WCHAR
 
 typedef __WCHAR_TYPE__ wchar_t;
 
Index: test/Preprocessor/wchar_t.c
===
--- /dev/null
+++ test/Preprocessor/wchar_t.c
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -triple i386-pc-solaris -dM -E %s -o - | FileCheck %s -check-prefix CHECK-SOLARIS
+// CHECK-SOLARIS-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-SOLARIS-DAG: #define __WCHAR_TYPE__ int
+// CHECK-SOLARIS-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple avr-unknown-unknown -fwchar-type=int -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-AVR
+// CHECK-AVR-DAG: #define __WCHAR_MAX__ 32767
+// CHECK-AVR-DAG: #define __WCHAR_TYPE__ int
+// CHECK-AVR-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple arm-unknown-none-gnu -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM-APCS
+// CHECK-ARM-APCS-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM-APCS-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM-APCS-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple arm-unknown-netbsd-gnu -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM-NETBSD-AAPCS
+// CHECK-ARM-NETBSD-AAPCS-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM-NETBSD-AAPCS-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM-NETBSD-AAPCS-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple arm-unknown-openbsd -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM-OPENBSD
+// CHECK-ARM-OPENBSD-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM-OPENBSD-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM-OPENBSD-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple arm64-apple-ios -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM64-DARWIN
+// CHECK-ARM64-DARWIN-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM64-DARWIN-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM64-DARWIN-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple aarch64-unknown-netbsd -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM64-NETBSD
+// CHECK-ARM64-NETBSD-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM64-NETBSD-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM64-NETBSD-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple aarch64-unknown-openbsd -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM64-OPENBSD
+// CHECK-ARM64-OPENBSD-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM64-OPENBSD-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM64-OPENBSD-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple aarch64-unknown-none -fwchar-type=int -fno-signed-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM64-AAPCS64
+// CHECK-ARM64-AAPCS64-DAG: #define __WCHAR_MAX__ 4294967295U
+// CHECK-ARM64-AAPCS64-DAG: #define __WCHAR_TYPE__ unsigned int
+// CHECK-ARM64-AAPCS64-DAG: #define __WCHAR_UNSIGNED__ 1
+
+// RUN: %clang_cc1 -triple xcore-unknown-unknown 

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: lib/Basic/TargetInfo.cpp:29
+namespace {
+TargetInfo::IntType GetDefaultWCharType(const llvm::Triple ) {
+  const llvm::Triple::ArchType Arch = T.getArch();

compnerd wrote:
> rnk wrote:
> > How is this better than what we had before? It's totally inconsistent with 
> > our existing strategy for figuring out type widths and sizes. Our current 
> > approach can be extended for new targets, this requires modifying shared 
> > TargetInfo code. This refactoring *should* be NFC anyway, so if we did want 
> > to do it, we'd want to split it out.
> The previous thing was split across and was fairly difficult to identify what 
> was going on.  I tend to think that this makes it easier to identify what is 
> going on.  However, if you have a strong opinion on this, Im willing to 
> switch it back (though, possibly retain some of the adjustments to make it 
> easier to follow).
I do, I want to see the minimal functional change. It'll make it easier to spot 
awkward places where we duplicate logic.


https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: lib/Basic/TargetInfo.cpp:29
+namespace {
+TargetInfo::IntType GetDefaultWCharType(const llvm::Triple ) {
+  const llvm::Triple::ArchType Arch = T.getArch();

rnk wrote:
> How is this better than what we had before? It's totally inconsistent with 
> our existing strategy for figuring out type widths and sizes. Our current 
> approach can be extended for new targets, this requires modifying shared 
> TargetInfo code. This refactoring *should* be NFC anyway, so if we did want 
> to do it, we'd want to split it out.
The previous thing was split across and was fairly difficult to identify what 
was going on.  I tend to think that this makes it easier to identify what is 
going on.  However, if you have a strong opinion on this, Im willing to switch 
it back (though, possibly retain some of the adjustments to make it easier to 
follow).


https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: lib/Basic/TargetInfo.cpp:29
+namespace {
+TargetInfo::IntType GetDefaultWCharType(const llvm::Triple ) {
+  const llvm::Triple::ArchType Arch = T.getArch();

How is this better than what we had before? It's totally inconsistent with our 
existing strategy for figuring out type widths and sizes. Our current approach 
can be extended for new targets, this requires modifying shared TargetInfo 
code. This refactoring *should* be NFC anyway, so if we did want to do it, we'd 
want to split it out.


https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 117832.
compnerd added a comment.

Moves the logic back into Basic.  The flags are now optional, but controlled by 
the driver.  The test adjustments are to map the old `-fshort-wchar` to 
`-fwchar-type=short -fno-signed-wchar` for the most part, there is one case 
where we were checking that we were passing `-fshort-wchar` through to the 
frontend, which we no longer do.


https://reviews.llvm.org/D37891

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/Basic/LangOptions.def
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets/AArch64.cpp
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/AVR.h
  lib/Basic/Targets/OSTargets.h
  lib/Basic/Targets/X86.h
  lib/Basic/Targets/XCore.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CXX/conv/conv.prom/p2.cpp
  test/CodeGen/arm-metadata.c
  test/CodeGen/pascal-wchar-string.c
  test/CodeGen/string-literal-short-wstring.c
  test/CodeGen/string-literal-unicode-conversion.c
  test/CodeGen/wchar-size.c
  test/Driver/clang_f_opts.c
  test/Headers/wchar_limits.cpp
  test/Index/index-pch.cpp
  test/Lexer/wchar.c
  test/Preprocessor/init.c
  test/Preprocessor/pr19649-unsigned-wchar_t.c
  test/Preprocessor/wchar_t.c
  test/Sema/wchar.c
  test/SemaCXX/short-wchar-sign.cpp

Index: test/SemaCXX/short-wchar-sign.cpp
===
--- test/SemaCXX/short-wchar-sign.cpp
+++ test/SemaCXX/short-wchar-sign.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple i386-mingw32 -fsyntax-only -pedantic -verify %s
-// RUN: %clang_cc1 -fshort-wchar -fsyntax-only -pedantic -verify %s
+// RUN: %clang_cc1 -fwchar-type=short -fno-signed-wchar -fsyntax-only -pedantic -verify %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -pedantic -verify %s
 // expected-no-diagnostics

Index: test/Sema/wchar.c
===
--- test/Sema/wchar.c
+++ test/Sema/wchar.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// RUN: %clang_cc1 %s -fsyntax-only -fshort-wchar -verify -DSHORT_WCHAR
+// RUN: %clang_cc1 %s -fsyntax-only -fwchar-type=short -fno-signed-wchar -verify -DSHORT_WCHAR
 
 typedef __WCHAR_TYPE__ wchar_t;
 
Index: test/Preprocessor/wchar_t.c
===
--- /dev/null
+++ test/Preprocessor/wchar_t.c
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -triple i386-pc-solaris -dM -E %s -o - | FileCheck %s -check-prefix CHECK-SOLARIS
+// CHECK-SOLARIS-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-SOLARIS-DAG: #define __WCHAR_TYPE__ int
+// CHECK-SOLARIS-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple avr-unknown-unknown -fwchar-type=int -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-AVR
+// CHECK-AVR-DAG: #define __WCHAR_MAX__ 32767
+// CHECK-AVR-DAG: #define __WCHAR_TYPE__ int
+// CHECK-AVR-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple arm-unknown-none-gnu -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM-APCS
+// CHECK-ARM-APCS-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM-APCS-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM-APCS-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple arm-unknown-netbsd-gnu -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM-NETBSD-AAPCS
+// CHECK-ARM-NETBSD-AAPCS-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM-NETBSD-AAPCS-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM-NETBSD-AAPCS-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple arm-unknown-openbsd -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM-OPENBSD
+// CHECK-ARM-OPENBSD-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM-OPENBSD-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM-OPENBSD-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple arm64-apple-ios -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM64-DARWIN
+// CHECK-ARM64-DARWIN-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM64-DARWIN-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM64-DARWIN-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple aarch64-unknown-netbsd -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM64-NETBSD
+// CHECK-ARM64-NETBSD-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM64-NETBSD-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM64-NETBSD-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple aarch64-unknown-openbsd -fsigned-wchar -dM -E %s -o - | FileCheck %s -check-prefix CHECK-ARM64-OPENBSD
+// CHECK-ARM64-OPENBSD-DAG: #define __WCHAR_MAX__ 2147483647
+// CHECK-ARM64-OPENBSD-DAG: #define __WCHAR_TYPE__ int
+// CHECK-ARM64-OPENBSD-NOT: #define __WCHAR_UNSIGNED__ 0
+
+// RUN: %clang_cc1 -triple aarch64-unknown-none -fwchar-type=int -fno-signed-wchar -dM -E %s -o - | 

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Matthias Braun via Phabricator via cfe-commits]

MatzeB added inline comments.



Comment at: lib/CodeGen/CodeGenModule.cpp:477
   Context.getTypeSizeInChars(Context.getWideCharType()).getQuantity();
-  assert((LangOpts.ShortWChar ||
-  llvm::TargetLibraryInfoImpl::getTargetWCharSize(Target.getTriple()) 
==

compnerd wrote:
> compnerd wrote:
> > MatzeB wrote:
> > > rnk wrote:
> > > > @MatzeB ptal
> > > Can you find a new place for this assert()? Please do not just remove it!
> > > 
> > > For the backstory: Unfortunately I had to duplicate the wchar decision 
> > > logic inside llvm (TargetLibraryInfoImpl::getTargetWCharSize() for cases 
> > > where we just have the target triple available but need to know the size 
> > > of wchar_t using library function. This means the logic in LLVM needs to 
> > > be updated when support for new platforms is added but for people adding 
> > > platform support it will not be obvious that they have the change 
> > > LLVM/TargetLibraryInfo as well unless an assert() point them to there 
> > > being a mismatch.
> > Sure, I'll try to see if I can find a suitable place or adjustment of it.  
> > However, one thing to note is that the frontend does actually embed that 
> > into the IR metadata ("wchar_size"). 
> I think that if we try to retain this assertion, we need to update all the 
> tests to ensure that they pass the arguments for selecting the `wchar_t` 
> type.  The entire problem is that the backend view of this cannot correlate 
> with what the user specified without passing it back to it.  The "wchar_size" 
> metadata does exactly that.  Using that to perform the validation for the 
> library function call.
This has been resolved with r314187 that made the assertion obsolete.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: lib/CodeGen/CodeGenModule.cpp:477
   Context.getTypeSizeInChars(Context.getWideCharType()).getQuantity();
-  assert((LangOpts.ShortWChar ||
-  llvm::TargetLibraryInfoImpl::getTargetWCharSize(Target.getTriple()) 
==

compnerd wrote:
> MatzeB wrote:
> > rnk wrote:
> > > @MatzeB ptal
> > Can you find a new place for this assert()? Please do not just remove it!
> > 
> > For the backstory: Unfortunately I had to duplicate the wchar decision 
> > logic inside llvm (TargetLibraryInfoImpl::getTargetWCharSize() for cases 
> > where we just have the target triple available but need to know the size of 
> > wchar_t using library function. This means the logic in LLVM needs to be 
> > updated when support for new platforms is added but for people adding 
> > platform support it will not be obvious that they have the change 
> > LLVM/TargetLibraryInfo as well unless an assert() point them to there being 
> > a mismatch.
> Sure, I'll try to see if I can find a suitable place or adjustment of it.  
> However, one thing to note is that the frontend does actually embed that into 
> the IR metadata ("wchar_size"). 
I think that if we try to retain this assertion, we need to update all the 
tests to ensure that they pass the arguments for selecting the `wchar_t` type.  
The entire problem is that the backend view of this cannot correlate with what 
the user specified without passing it back to it.  The "wchar_size" metadata 
does exactly that.  Using that to perform the validation for the library 
function call.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: lib/CodeGen/CodeGenModule.cpp:477
   Context.getTypeSizeInChars(Context.getWideCharType()).getQuantity();
-  assert((LangOpts.ShortWChar ||
-  llvm::TargetLibraryInfoImpl::getTargetWCharSize(Target.getTriple()) 
==

MatzeB wrote:
> rnk wrote:
> > @MatzeB ptal
> Can you find a new place for this assert()? Please do not just remove it!
> 
> For the backstory: Unfortunately I had to duplicate the wchar decision logic 
> inside llvm (TargetLibraryInfoImpl::getTargetWCharSize() for cases where we 
> just have the target triple available but need to know the size of wchar_t 
> using library function. This means the logic in LLVM needs to be updated when 
> support for new platforms is added but for people adding platform support it 
> will not be obvious that they have the change LLVM/TargetLibraryInfo as well 
> unless an assert() point them to there being a mismatch.
Sure, I'll try to see if I can find a suitable place or adjustment of it.  
However, one thing to note is that the frontend does actually embed that into 
the IR metadata ("wchar_size"). 


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Matthias Braun via Phabricator via cfe-commits]

MatzeB added inline comments.



Comment at: lib/CodeGen/CodeGenModule.cpp:477
   Context.getTypeSizeInChars(Context.getWideCharType()).getQuantity();
-  assert((LangOpts.ShortWChar ||
-  llvm::TargetLibraryInfoImpl::getTargetWCharSize(Target.getTriple()) 
==

rnk wrote:
> @MatzeB ptal
Can you find a new place for this assert()? Please do not just remove it!

For the backstory: Unfortunately I had to duplicate the wchar decision logic 
inside llvm (TargetLibraryInfoImpl::getTargetWCharSize() for cases where we 
just have the target triple available but need to know the size of wchar_t 
using library function. This means the logic in LLVM needs to be updated when 
support for new platforms is added but for people adding platform support it 
will not be obvious that they have the change LLVM/TargetLibraryInfo as well 
unless an assert() point them to there being a mismatch.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 115510.
compnerd added a comment.

Try to clarify the logic for APCS ABI.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/Basic/LangOptions.def
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets/AArch64.cpp
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/AVR.h
  lib/Basic/Targets/OSTargets.h
  lib/Basic/Targets/X86.h
  lib/Basic/Targets/XCore.h
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CXX/conv/conv.prom/p2.cpp
  test/CodeGen/aarch64-type-sizes.c
  test/CodeGen/arm-metadata.c
  test/CodeGen/coff-aarch64-type-sizes.c
  test/CodeGen/ms-annotation.c
  test/CodeGen/pascal-wchar-string.c
  test/CodeGen/string-literal-short-wstring.c
  test/CodeGen/string-literal-unicode-conversion.c
  test/CodeGen/wchar-const.c
  test/CodeGen/wchar-size.c
  test/CodeGenCXX/mangle-ms-string-literals.cpp
  test/CodeGenCXX/ms_wide_predefined_expr.cpp
  test/Driver/clang_f_opts.c
  test/Driver/rewrite-legacy-objc.m
  test/Driver/rewrite-objc.m
  test/Driver/wchar_t.c
  test/Headers/wchar_limits.cpp
  test/Index/index-pch.cpp
  test/Lexer/wchar-signedness.c
  test/Lexer/wchar.c
  test/Preprocessor/init.c
  test/Preprocessor/pr19649-unsigned-wchar_t.c
  test/Preprocessor/stdint.c
  test/Preprocessor/wchar_t.c
  test/Preprocessor/woa-defaults.c
  test/Preprocessor/woa-wchar_t.c
  test/Sema/format-strings-ms.c
  test/Sema/wchar.c
  test/SemaCXX/no-wchar.cpp
  test/SemaCXX/short-wchar-sign.cpp

Index: test/SemaCXX/short-wchar-sign.cpp
===
--- test/SemaCXX/short-wchar-sign.cpp
+++ test/SemaCXX/short-wchar-sign.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple i386-mingw32 -fsyntax-only -pedantic -verify %s
-// RUN: %clang_cc1 -fshort-wchar -fsyntax-only -pedantic -verify %s
+// RUN: %clang_cc1 -fwchar-type=short -fno-signed-wchar -fsyntax-only -pedantic -verify %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -pedantic -verify %s
 // expected-no-diagnostics
 
Index: test/SemaCXX/no-wchar.cpp
===
--- test/SemaCXX/no-wchar.cpp
+++ test/SemaCXX/no-wchar.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fno-wchar -verify %s
-// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fno-wchar -verify -std=c++98 %s
-// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fno-wchar -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fwchar-type=short -fno-signed-wchar -fno-wchar -verify %s
+// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fwchar-type=short -fno-signed-wchar -fno-wchar -verify -std=c++98 %s
+// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fwchar-type=short -fno-signed-wchar -fno-wchar -verify -std=c++11 %s
 wchar_t x; // expected-error {{unknown type name 'wchar_t'}}
 
 typedef unsigned short wchar_t;
Index: test/Sema/wchar.c
===
--- test/Sema/wchar.c
+++ test/Sema/wchar.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// RUN: %clang_cc1 %s -fsyntax-only -fshort-wchar -verify -DSHORT_WCHAR
+// RUN: %clang_cc1 %s -fsyntax-only -fwchar-type=short -fno-signed-wchar -verify -DSHORT_WCHAR
 
 typedef __WCHAR_TYPE__ wchar_t;
 
Index: test/Sema/format-strings-ms.c
===
--- test/Sema/format-strings-ms.c
+++ test/Sema/format-strings-ms.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 %s
-// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -fwchar-type=short -fno-signed-wchar -triple=i386-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -fwchar-type=short -fno-signed-wchar -triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
 
 int printf(const char *format, ...) __attribute__((format(printf, 1, 2)));
 int scanf(const char * restrict, ...) ;
Index: test/Preprocessor/woa-wchar_t.c
===
--- test/Preprocessor/woa-wchar_t.c
+++ test/Preprocessor/woa-wchar_t.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -dM -triple armv7-windows -E %s | FileCheck %s
-// RUN: %clang_cc1 -dM -fno-signed-char -triple armv7-windows -E %s | FileCheck %s
+// RUN: %clang_cc1 -dM -fwchar-type=short -fno-signed-wchar -triple armv7-windows -E %s | FileCheck %s
+// RUN: %clang_cc1 -dM -fwchar-type=short -fno-signed-wchar -fno-signed-char -triple armv7-windows -E %s | FileCheck %s
 
 // CHECK: #define __WCHAR_TYPE__ unsigned short
 
Index: test/Preprocessor/woa-defaults.c

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:2659
+
+  const bool IsAPCSABI =
+  IsARM && (IsGNUEnvironment || IsNetBSD ||

rnk wrote:
> This target detection logic is insanely complicated, and I'm not convinced 
> it's simpler than the Basic/Targets/ delegation strategy.
> 
> I can go either way on putting this info in the Driver or Basic, but this 
> code needs to be simpler. Surely a switch could help here?
I like the idea of the switch, I'll try to convert this.  In my defense, this 
was trying to be a translation of the original conditions in Basic.  Doesn't 
mean that I like this or don't want this to converted into something saner.

If we put this into Basic, we get back the 3-bits of information ({`char`, 
`short`, `int`} * {`signed`, `unsigned`}) into 1-bit :-p.  This was a pretty 
painful change, but I think that it does result in a simpler flow for the 
frontend.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: lib/CodeGen/CodeGenModule.cpp:477
   Context.getTypeSizeInChars(Context.getWideCharType()).getQuantity();
-  assert((LangOpts.ShortWChar ||
-  llvm::TargetLibraryInfoImpl::getTargetWCharSize(Target.getTriple()) 
==

@MatzeB ptal



Comment at: lib/Driver/ToolChains/Clang.cpp:2659
+
+  const bool IsAPCSABI =
+  IsARM && (IsGNUEnvironment || IsNetBSD ||

This target detection logic is insanely complicated, and I'm not convinced it's 
simpler than the Basic/Targets/ delegation strategy.

I can go either way on putting this info in the Driver or Basic, but this code 
needs to be simpler. Surely a switch could help here?


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

We could leave the defaults there as they stand, and only have the new flags 
alter the default.  However, it seems that just paying the cost of adjusting 
the tests once isn't too bad.  To me, it seems that having one instance of the 
horrible logic for determining the type of `wchar_t` is better than having two 
copies which can diverge slightly.  My understanding was that we determined 
that it was better to just pay this test adjustment cost, since it would treat 
all the targets the same.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I do remember recommending this approach over IRC, but I thought we concluded 
that we should leave all the defaults in lib/Basic/Targets/ and make the -cc1 
-fwchar-type= and -f[no-]signed-wchar overrides that affected all targets 
equally. That would avoid the need for these test updates, anyway.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891



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

[PATCH] D37891: Driver: hoist the `wchar_t` handling to the driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added a project: clang.
Herald added subscribers: fedor.sergeev, javed.absar.

Move the logic for determining the `wchar_t` type information into the
driver.  Rather than passing the single bit of information of
`-fshort-wchar` indicate to the frontend the desired type of `wchar_t`
through a new `-cc1` option of `-fwchar-type` and indicate the
signedness through `-f{,no-}signed-wchar`.  This replicates the current
logic which was spread throughout Basic into the
`RenderCharacterOptions`.

Most of the changes to the tests are to ensure that the frontend uses
the correct type.  Add a new test set under `test/Driver/wchar_t.c` to
ensure that we calculate the proper types for the various cases.


Repository:
  rL LLVM

https://reviews.llvm.org/D37891

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/Basic/LangOptions.def
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets/AArch64.cpp
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/AVR.h
  lib/Basic/Targets/OSTargets.h
  lib/Basic/Targets/X86.h
  lib/Basic/Targets/XCore.h
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CXX/conv/conv.prom/p2.cpp
  test/CodeGen/aarch64-type-sizes.c
  test/CodeGen/arm-metadata.c
  test/CodeGen/coff-aarch64-type-sizes.c
  test/CodeGen/ms-annotation.c
  test/CodeGen/pascal-wchar-string.c
  test/CodeGen/string-literal-short-wstring.c
  test/CodeGen/string-literal-unicode-conversion.c
  test/CodeGen/wchar-const.c
  test/CodeGen/wchar-size.c
  test/CodeGenCXX/mangle-ms-string-literals.cpp
  test/CodeGenCXX/ms_wide_predefined_expr.cpp
  test/Driver/clang_f_opts.c
  test/Driver/rewrite-legacy-objc.m
  test/Driver/rewrite-objc.m
  test/Driver/wchar_t.c
  test/Headers/wchar_limits.cpp
  test/Index/index-pch.cpp
  test/Lexer/wchar-signedness.c
  test/Lexer/wchar.c
  test/Preprocessor/init.c
  test/Preprocessor/pr19649-unsigned-wchar_t.c
  test/Preprocessor/stdint.c
  test/Preprocessor/wchar_t.c
  test/Preprocessor/woa-defaults.c
  test/Preprocessor/woa-wchar_t.c
  test/Sema/format-strings-ms.c
  test/Sema/wchar.c
  test/SemaCXX/no-wchar.cpp
  test/SemaCXX/short-wchar-sign.cpp

Index: test/SemaCXX/short-wchar-sign.cpp
===
--- test/SemaCXX/short-wchar-sign.cpp
+++ test/SemaCXX/short-wchar-sign.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple i386-mingw32 -fsyntax-only -pedantic -verify %s
-// RUN: %clang_cc1 -fshort-wchar -fsyntax-only -pedantic -verify %s
+// RUN: %clang_cc1 -fwchar-type=short -fno-signed-wchar -fsyntax-only -pedantic -verify %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -pedantic -verify %s
 // expected-no-diagnostics
 
Index: test/SemaCXX/no-wchar.cpp
===
--- test/SemaCXX/no-wchar.cpp
+++ test/SemaCXX/no-wchar.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fno-wchar -verify %s
-// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fno-wchar -verify -std=c++98 %s
-// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fno-wchar -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fwchar-type=short -fno-signed-wchar -fno-wchar -verify %s
+// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fwchar-type=short -fno-signed-wchar -fno-wchar -verify -std=c++98 %s
+// RUN: %clang_cc1 -triple i386-pc-win32 -fsyntax-only -fwchar-type=short -fno-signed-wchar -fno-wchar -verify -std=c++11 %s
 wchar_t x; // expected-error {{unknown type name 'wchar_t'}}
 
 typedef unsigned short wchar_t;
Index: test/Sema/wchar.c
===
--- test/Sema/wchar.c
+++ test/Sema/wchar.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify
-// RUN: %clang_cc1 %s -fsyntax-only -fshort-wchar -verify -DSHORT_WCHAR
+// RUN: %clang_cc1 %s -fsyntax-only -fwchar-type=short -fno-signed-wchar -verify -DSHORT_WCHAR
 
 typedef __WCHAR_TYPE__ wchar_t;
 
Index: test/Sema/format-strings-ms.c
===
--- test/Sema/format-strings-ms.c
+++ test/Sema/format-strings-ms.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 %s
-// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -fwchar-type=short -fno-signed-wchar -triple=i386-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -fwchar-type=short -fno-signed-wchar -triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
 
 int printf(const char *format, ...) __attribute__((format(printf, 1, 2)));
 int scanf(const char * restrict, ...) ;
Index: test/Preprocessor/woa-wchar_t.c