[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: lib/Headers/__stddef_max_align_t.h:40
   __attribute__((__aligned__(__alignof__(long double;
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3

Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in 
InitPreprocessor alongside the rest of the SIZEOF macros?

And then use that to determine whether to add float128 to the union? This 
change, as is, will break on any target which is i386 but doesn't define 
__float128, e.g. i386-unknown-unknown.

The only additional target which will be modified with that (that is: the only 
other target which has a float128 type, but for which max_align isn't already 
16) is systemz-*-linux.

But I think that's actually indicating a pre-existing bug in the SystemZ config 
-- it's configured for a 16-byte long double, with 8-byte alignment, but the 
ABI doc seems to call for 16-byte alignment. +Ulrich for comment on that.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55057



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

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: lib/Headers/__stddef_max_align_t.h:40
   __attribute__((__aligned__(__alignof__(long double;
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3

uweigand wrote:
> jyknight wrote:
> > Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in 
> > InitPreprocessor alongside the rest of the SIZEOF macros?
> > 
> > And then use that to determine whether to add float128 to the union? This 
> > change, as is, will break on any target which is i386 but doesn't define 
> > __float128, e.g. i386-unknown-unknown.
> > 
> > The only additional target which will be modified with that (that is: the 
> > only other target which has a float128 type, but for which max_align isn't 
> > already 16) is systemz-*-linux.
> > 
> > But I think that's actually indicating a pre-existing bug in the SystemZ 
> > config -- it's configured for a 16-byte long double, with 8-byte alignment, 
> > but the ABI doc seems to call for 16-byte alignment. +Ulrich for comment on 
> > that.
> That's a bug in the ABI doc which we'll fix once we get around to releasing 
> an updated version.
> 
> long double on SystemZ must be 8-byte aligned, which is the maximum alignment 
> of all standard types on Z, and this was chosen because historically the ABI 
> only guarantees an 8-byte stack alignment, so 16-byte aligned standard types 
> would be awkward.
Then perhaps it's a bug that `__alignof__(__float128)` returns 16 with `-target 
systemz-linux`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55057



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: rsmith, chandlerc.
Herald added a subscriber: arphaman.

This warning, Wexperimental-driver-option, is on by default, exempt
from -Werror, and may be disabled.

Additionally, change the documentation to note that these options are
not intended for common usage.


https://reviews.llvm.org/D55150

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3419,6 +3419,11 @@
   if (options & CXTranslationUnit_KeepGoing)
 Diags->setSuppressAfterFatalError(false);
 
+  // Suppress driver warning for use of -Xclang, since we add it ourselves.
+  Diags->setSeverityForGroup(diag::Flavor::WarningOrError,
+ "experimental-driver-option",
+ diag::Severity::Ignored, SourceLocation());
+
   // Recover resources if we crash before exiting this function.
   llvm::CrashRecoveryContextCleanupRegistrar >
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4994,6 +4994,11 @@
 
   // Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option
   // parser.
+  if (Args.hasArg(options::OPT_Xclang))
+D.Diag(diag::warn_drv_xclang_option);
+  if (Args.hasArg(options::OPT_mllvm))
+D.Diag(diag::warn_drv_mllvm_option);
+
   // -finclude-default-header flag is for preprocessor,
   // do not pass it to other cc1 commands when save-temps is enabled
   if (C.getDriver().isSaveTempsEnabled() &&
@@ -5926,6 +5931,8 @@
   CollectArgsForIntegratedAssembler(C, Args, CmdArgs,
 getToolChain().getDriver());
 
+  if (Args.hasArg(options::OPT_mllvm))
+D.Diag(diag::warn_drv_mllvm_option);
   Args.AddAllArgs(CmdArgs, options::OPT_mllvm);
 
   assert(Output.isFilename() && "Unexpected lipo output.");
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -466,7 +466,7 @@
   HelpText<"Pass  to the assembler">, MetaVarName<"">,
   Group;
 def Xclang : Separate<["-"], "Xclang">,
-  HelpText<"Pass  to the clang compiler">, MetaVarName<"">,
+  HelpText<"Pass  to the clang cc1 frontend. For experimental use only">, MetaVarName<"">,
   Flags<[DriverOption, CoreOption]>, Group;
 def Xcuda_fatbinary : Separate<["-"], "Xcuda-fatbinary">,
   HelpText<"Pass  to fatbinary invocation">, MetaVarName<"">;
@@ -2002,7 +2002,7 @@
 def mlinker_version_EQ : Joined<["-"], "mlinker-version=">,
   Flags<[DriverOption]>;
 def mllvm : Separate<["-"], "mllvm">, Flags<[CC1Option,CC1AsOption,CoreOption]>,
-  HelpText<"Additional arguments to forward to LLVM's option processing">;
+  HelpText<"Additional arguments to forward to LLVM's option processing. For experimental use only">;
 def mmacosx_version_min_EQ : Joined<["-"], "mmacosx-version-min=">,
   Group, HelpText<"Set Mac OS X deployment target">;
 def mmacos_version_min_EQ : Joined<["-"], "mmacos-version-min=">,
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -1039,3 +1039,7 @@
 // A warning group specifically for warnings related to function
 // multiversioning.
 def FunctionMultiVersioning : DiagGroup<"function-multiversion">;
+
+// Warning for command-line options that are not a supported part of the clang command-line interface.
+// Warnings in this group should be on by default, and exempt from -Werror.
+def ExperimentalDriverOption : DiagGroup<"experimental-driver-option">;
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -405,4 +405,14 @@
 def warn_drv_moutline_unsupported_opt : Warning<
   "The '%0' architecture does not support -moutline; flag ignored">,
   InGroup;
+
+def warn_drv_xclang_option : Warning<
+  "-Xclang options are for experimental use only; future compatibility may not be preserved">,
+  InGroup, DefaultWarnNoWerror;
+
+def warn_drv_mllvm_option : Warning<
+  "-mllvm options are for experimental use only; future compatibility may not be preserved">,
+  InGroup, DefaultWarnNoWerror;
+
 }
+
Index: clang/docs/UsersManual.rst
===
--- clang/docs/User

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: lib/Headers/__stddef_max_align_t.h:40
   __attribute__((__aligned__(__alignof__(long double;
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3

EricWF wrote:
> uweigand wrote:
> > jyknight wrote:
> > > uweigand wrote:
> > > > jyknight wrote:
> > > > > Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in 
> > > > > InitPreprocessor alongside the rest of the SIZEOF macros?
> > > > > 
> > > > > And then use that to determine whether to add float128 to the union? 
> > > > > This change, as is, will break on any target which is i386 but 
> > > > > doesn't define __float128, e.g. i386-unknown-unknown.
> > > > > 
> > > > > The only additional target which will be modified with that (that is: 
> > > > > the only other target which has a float128 type, but for which 
> > > > > max_align isn't already 16) is systemz-*-linux.
> > > > > 
> > > > > But I think that's actually indicating a pre-existing bug in the 
> > > > > SystemZ config -- it's configured for a 16-byte long double, with 
> > > > > 8-byte alignment, but the ABI doc seems to call for 16-byte 
> > > > > alignment. +Ulrich for comment on that.
> > > > That's a bug in the ABI doc which we'll fix once we get around to 
> > > > releasing an updated version.
> > > > 
> > > > long double on SystemZ must be 8-byte aligned, which is the maximum 
> > > > alignment of all standard types on Z, and this was chosen because 
> > > > historically the ABI only guarantees an 8-byte stack alignment, so 
> > > > 16-byte aligned standard types would be awkward.
> > > Then perhaps it's a bug that `__alignof__(__float128)` returns 16 with 
> > > `-target systemz-linux`?
> > Huh, really __float128 should not be supported at all on SystemZ.  It's not 
> > supported with GCC either (GCC never provides __float128 on targets where 
> > long double is already IEEE-128).  (GCC does support the new _Float128 on 
> > SystemZ, but that's not yet supported by clang anywhere.)
> > 
> > If it were supported, I agree it should be an alias for long double, and 
> > also have an alignof of 8.
> @jyknight Ack. I'll add `__SIZEOF_FLOAT128__`.
@uweigand : One of those fixes needs to land before this, so that systemz's 
max_align_t doesn't change to 16 in the meantime. I think your preference would 
be for it to be simply removed, right? Looks like the type was originally added 
in https://reviews.llvm.org/D19125 -- possibly in error, since the focus was 
x86_64.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55057



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

[PATCH] D54547: PTH-- Remove feature entirely-

[James Y Knight via Phabricator via cfe-commits]

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

Since you've already submitted a fix to Boost, 
https://github.com/boostorg/build/commit/3385fe2aa699a45e722a1013658f824b6a7c761f,
 I think this is fine to remove.


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

https://reviews.llvm.org/D54547



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1318082 , @chandlerc wrote:

> I think this should be `internal-driver-option` and the text updated? I don't 
> think these are necessarily experimental, they're internals of the compiler 
> implementation, and not a supported interface for users. Unsure how to best 
> explain this.


Yea, I was trying to figure out a good name. I thought "unsupported" at first, 
but that is a tricky word, since it has the connotation of "doesn't work in 
this version", which is not what I mean to imply -- the thing someone wants to 
do likely does work, but we need to indicate no guarantees about the future. 
Which is where "experimental" came from -- the options are useful for 
experimenting with compiler features that are not fully baked.

Perhaps "internal" is okay too...


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321046 , @kristina wrote:

> Personally I'm against this type of warning as it's likely anyone using 
> `-mllvm` is actually intending to adjust certain behaviors across one or more 
> passes with a lot of switches supported by it being intentionally hidden from 
> ` --help` output requiring explicitly specifying that hidden flags 
> be shown.


There is a cost to having people encode these flags into their build systems -- 
it can then cause issues if we ever change these internal flags. I do not think 
any Clang maintainer intends to support these as stable APIs, unlike most of 
the driver command-line. But, neither -Xclang nor -mllvm obviously scream out 
"don't use this!", and so people may then add them to their buildsystems 
without causing reviewers to say "Wait...really? Are you sure that's a good 
idea?".

That's why I think a warning is useful -- it'll discourage people from using 
them when they haven't properly understand the consequences, but does not 
prevent them from doing so, when they actually do.

> For example, I routinely use the following with SEH (excuse some of the 
> naming, this is just a downstream fork however):
>  `-mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm 
> -wtfabi-opts=0x1EF77F`

If you already are passing that, do you see a problem with instead passing
 `-mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm 
-wtfabi-opts=0x1EF77F -Wno-experimental-driver-option`
?


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321705 , @kristina wrote:

> > There is a cost to having people encode these flags into their build 
> > systems -- it can then cause issues if we ever change these internal flags. 
> > I do not think any Clang maintainer intends to support these as stable 
> > APIs, unlike most of the driver command-line. But, neither -Xclang nor 
> > -mllvm obviously scream out "don't use this!", and so people may then add 
> > them to their buildsystems without causing reviewers to say "Wait...really? 
> > Are you sure that's a good idea?".
>
> This is why I proposed a compromise, allow this warning to be disabled 
> completely for people actively using those flags, which are pretty much 
> exclusively toolchain developers (well basically what I proposed, since it's 
> not clear what counts as a build made by someone who is working and debugging 
> a pass, being fully aware of those flags, using the subset of them specific 
> to that pass/feature, I would say assertion+dump enabled builds are closest, 
> but having an explicit build flag for it would be nicer). It's more unified 
> than everyone either adding workarounds into build systems or disabling it 
> completely (by just removing it).


I mean, I'm not much opposed to adding that -- just that any new build-time 
options is always a minor shame. But you didn't respond to the other part of my 
message -- is adding `-Wno-experimental-driver-option` to your compile-flags 
not already a completely sufficient solution for your use-case?

> Besides, I don't think this really ever surfaced as an issue, until 
> Chandler's idea with regards to an unsupressable warning for performance 
> tests for 7.x.x

The primary impetus for me was actually the discovery that Boost's build system 
was using "-Xclang -include-pth" a few weeks back.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321759 , @george.karpenkov 
wrote:

> Using `-Xclang` is the only way to pass options to the static analyzer, I 
> don't think we should warn on it.


Well,, that seems unfortunate if we have the only supported interface for the 
static analyzer be an internal interface. Perhaps it can be given a different 
option? Even discounting this change, I that seems like it would be appropriate.

In D55150#1321771 , @arphaman wrote:

> Swift uses `-Xclang` to pass in build settings to its own build and to pass 
> in custom options through its Clang importer that we intentionally don't want 
> to expose to Clang's users. We don't want to warn for those uses for sure.


I'm not sure if I understand correctly, so I'll try to reiterate: Swift calls 
clang internally, and when it does so, intentionally uses options that are not 
generally intended for others to use. Of course we shouldn't emit warnings in 
that case.

Is that a correct understanding? If so, doesn't it just make sense for that 
constructed command-line to disable the warning? And, isn't it good that it 
will warn, otherwise, in order to discourage people from using those flags that 
are intentionally-not-exposed?


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321829 , @efriedma wrote:

> I'm not sure that putting a warning that can be disabled really helps here; 
> anyone who needs the option will just disable the warning anyway, and then 
> users adding additional options somewhere else in the build system will miss 
> the warning.
>
> Instead, it would probably be better to rename Xclang and mllvm to something 
> that makes it clear the user is doing something unsupported. Maybe 
> "--unstable-llvm-option" and "--unstable-clang-option" or something like 
> that.  (This will lead to some breakage, but the breakage is roughly 
> equivalent for anyone using -Werror.)


Possibly you missed that this flag is exempt from -Werror -- it _only_ prints a 
warning, and is not an error, even when -Werror is specified.


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

https://reviews.llvm.org/D55150



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

[PATCH] D53199: Fix the behavior of clang's -w flag.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ping.


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

https://reviews.llvm.org/D53199



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

[PATCH] D57330: Adjust documentation for git migration.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: jlebar, rnk, mehdi_amini.
Herald added subscribers: jsji, jfb, arphaman, christof, delcypher, hiraditya, 
nhaehnle, jvesely, nemanjai, kubamracek, arsenm.
Herald added a reviewer: bollu.
Herald added a reviewer: serge-sans-paille.

This fixes most references to the paths:
 llvm.org/svn/
 llvm.org/git/
 llvm.org/viewvc/
 github.com/llvm-mirror/
 github.com/llvm-project/
 reviews.llvm.org/diffusion/

to instead point to https://github.com/llvm/llvm-project.

This is *not* a trivial substitution, because additionally, all the
checkout instructions had to be migrated to instruct users on how to
use the monorepo layout, setting LLVM_ENABLE_PROJECTS instead of
checking out various projects into various subdirectories.

I've attempted to *NOT* change any scripts here, only
documentation. The scripts will have to be addressed separately.


https://reviews.llvm.org/D57330

Files:
  clang-tools-extra/docs/clang-rename.rst
  clang-tools-extra/docs/clang-tidy/Contributing.rst
  clang-tools-extra/docs/clang-tidy/Integrations.rst
  clang/.gitignore
  clang/docs/ClangPlugins.rst
  clang/docs/ClangTools.rst
  clang/docs/ControlFlowIntegrityDesign.rst
  clang/docs/InternalsManual.rst
  clang/docs/LibASTMatchersTutorial.rst
  clang/docs/LibTooling.rst
  clang/docs/Toolchain.rst
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/www/analyzer/checker_dev_manual.html
  clang/www/get_started.html
  clang/www/hacking.html
  clang/www/menu.html.incl
  compiler-rt/include/sanitizer/tsan_interface_atomic.h
  compiler-rt/lib/tsan/rtl/tsan_interface.h
  compiler-rt/www/index.html
  compiler-rt/www/menu.html.incl
  libclc/www/index.html
  libcxx/docs/BuildingLibcxx.rst
  libcxx/docs/index.rst
  libcxx/www/TS_deprecation.html
  libcxx/www/atomic_design.html
  libcxx/www/atomic_design_a.html
  libcxx/www/atomic_design_b.html
  libcxx/www/atomic_design_c.html
  libcxx/www/cxx1y_status.html
  libcxx/www/cxx1z_status.html
  libcxx/www/cxx2a_status.html
  libcxx/www/index.html
  libcxx/www/ts1z_status.html
  libcxx/www/type_traits_design.html
  libcxx/www/upcoming_meeting.html
  libcxxabi/www/index.html
  libunwind/docs/BuildingLibunwind.rst
  libunwind/docs/index.rst
  lld/docs/getting_started.rst
  lld/docs/index.rst
  lldb/docs/building-with-debug-llvm.txt
  
lldb/packages/Python/lldbsuite/test/functionalities/command_source/TestCommandSource.py
  
lldb/packages/Python/lldbsuite/test/lang/objc/objc-optimized/TestObjcOptimized.py
  lldb/utils/vim-lldb/doc/lldb.txt
  lldb/www/adding-language-support.html
  lldb/www/build.html
  lldb/www/index.html
  lldb/www/python-reference.html
  lldb/www/scripting.html
  lldb/www/sidebar.incl
  lldb/www/source.html
  lldb/www/symbolication.html
  lldb/www/varformats.html
  llgo/README.TXT
  llvm/docs/CompileCudaWithLLVM.rst
  llvm/docs/LibFuzzer.rst
  llvm/docs/TestSuiteMakefileGuide.rst
  llvm/docs/TestingGuide.rst
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/Target/AMDGPU/AMDGPUISelLowering.h
  llvm/test/CodeGen/PowerPC/pr24546.ll
  llvm/utils/lit/setup.py
  openmp/www/index.html
  polly/docs/TipsAndTricks.rst
  polly/www/get_started.html
  polly/www/menu.html.incl
  polly/www/todo.html

Index: polly/www/todo.html
===
--- polly/www/todo.html
+++ polly/www/todo.html
@@ -411,12 +411,6 @@
 >http://llvm.org/svn/llvm-project/polly
  Tobias
 
-
-
- Git mirror
-
-git://llvm.org/git/polly.git
- Tobias
 
 
  Commit mails
Index: polly/www/menu.html.incl
===
--- polly/www/menu.html.incl
+++ polly/www/menu.html.incl
@@ -34,7 +34,7 @@
 http://lab.llvm.org:8080/coverage/coverage-reports/polly/index.html";>Code Coverage
 http://llvm.org/reports/scan-build/";>Static analysis
 Doxygen
-https://github.com/llvm-mirror/polly";>Source @ GitHub
+https://github.com/llvm/llvm-project/tree/master/polly";>Source @ GitHub
   
 
   
Index: polly/www/get_started.html
===
--- polly/www/get_started.html
+++ polly/www/get_started.html
@@ -33,20 +33,14 @@
  Manual 
  Get the code 
 
-Warning: Polly/LLVM/clang need to be checked out at the same time.
-
 
-git clone http://llvm.org/git/llvm.git llvm_git
-git clone http://llvm.org/git/polly.git llvm_git/tools/polly
-
-# Also build the matching clang-version (optional)
-git clone http://llvm.org/git/clang.git llvm_git/tools/clang
+git clone http://github.com/llvm/llvm-project.git llvm_git
 
 Build Polly
 
 
 mkdir llvm_build && cd llvm_build
-cmake ../llvm_git && make
+cmake -DLLVM_ENABLE_PROJECTS='polly;clang' ../llvm_git/llvm && make
 
 
  Test Polly
@@ -59,7 +53,7 @@
 build. To configure Polly to use a pre-built LLVM, set the
 -DCMAKE_PREFIX_PATH option:
 
-cmake -DCMAKE_PREFIX_PATH=${LLVM_PREFIX}/lib/cmake/llvm
+cmake -DCMAKE_PREFIX_PATH=${LLVM_PREFIX}/lib/cmake/llvm ../llvm_git/polly
 
 To run unittests,

[PATCH] D57330: Adjust documentation for git migration.

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 183891.
jyknight added a comment.

Fix some warnings I added.

Restore TestSuiteMakefileGuide.rst, which apparently isn't 100%
obsolete. (But it is incorrect, and I'm not sure exactly how to fix
it, so I just left a FIXME).


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

https://reviews.llvm.org/D57330

Files:
  clang-tools-extra/docs/clang-rename.rst
  clang-tools-extra/docs/clang-tidy/Contributing.rst
  clang-tools-extra/docs/clang-tidy/Integrations.rst
  clang/.gitignore
  clang/docs/ClangPlugins.rst
  clang/docs/ClangTools.rst
  clang/docs/ControlFlowIntegrityDesign.rst
  clang/docs/InternalsManual.rst
  clang/docs/LibASTMatchersTutorial.rst
  clang/docs/LibTooling.rst
  clang/docs/Toolchain.rst
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/www/analyzer/checker_dev_manual.html
  clang/www/get_started.html
  clang/www/hacking.html
  clang/www/menu.html.incl
  compiler-rt/include/sanitizer/tsan_interface_atomic.h
  compiler-rt/lib/tsan/rtl/tsan_interface.h
  compiler-rt/www/index.html
  compiler-rt/www/menu.html.incl
  libclc/www/index.html
  libcxx/docs/BuildingLibcxx.rst
  libcxx/docs/index.rst
  libcxx/www/TS_deprecation.html
  libcxx/www/atomic_design.html
  libcxx/www/atomic_design_a.html
  libcxx/www/atomic_design_b.html
  libcxx/www/atomic_design_c.html
  libcxx/www/cxx1y_status.html
  libcxx/www/cxx1z_status.html
  libcxx/www/cxx2a_status.html
  libcxx/www/index.html
  libcxx/www/ts1z_status.html
  libcxx/www/type_traits_design.html
  libcxx/www/upcoming_meeting.html
  libcxxabi/www/index.html
  libunwind/docs/BuildingLibunwind.rst
  libunwind/docs/index.rst
  lld/docs/getting_started.rst
  lld/docs/index.rst
  lldb/docs/building-with-debug-llvm.txt
  
lldb/packages/Python/lldbsuite/test/functionalities/command_source/TestCommandSource.py
  
lldb/packages/Python/lldbsuite/test/lang/objc/objc-optimized/TestObjcOptimized.py
  lldb/utils/vim-lldb/doc/lldb.txt
  lldb/www/adding-language-support.html
  lldb/www/build.html
  lldb/www/index.html
  lldb/www/python-reference.html
  lldb/www/scripting.html
  lldb/www/sidebar.incl
  lldb/www/source.html
  lldb/www/symbolication.html
  lldb/www/varformats.html
  llgo/README.TXT
  llvm/docs/CompileCudaWithLLVM.rst
  llvm/docs/LibFuzzer.rst
  llvm/docs/TestSuiteGuide.md
  llvm/docs/TestSuiteMakefileGuide.rst
  llvm/docs/TestingGuide.rst
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/Target/AMDGPU/AMDGPUISelLowering.h
  llvm/test/CodeGen/PowerPC/pr24546.ll
  llvm/utils/lit/setup.py
  openmp/www/index.html
  polly/docs/TipsAndTricks.rst
  polly/www/get_started.html
  polly/www/menu.html.incl
  polly/www/todo.html

Index: polly/www/todo.html
===
--- polly/www/todo.html
+++ polly/www/todo.html
@@ -411,12 +411,6 @@
 >http://llvm.org/svn/llvm-project/polly
  Tobias
 
-
-
- Git mirror
-
-git://llvm.org/git/polly.git
- Tobias
 
 
  Commit mails
Index: polly/www/menu.html.incl
===
--- polly/www/menu.html.incl
+++ polly/www/menu.html.incl
@@ -34,7 +34,7 @@
 http://lab.llvm.org:8080/coverage/coverage-reports/polly/index.html";>Code Coverage
 http://llvm.org/reports/scan-build/";>Static analysis
 Doxygen
-https://github.com/llvm-mirror/polly";>Source @ GitHub
+https://github.com/llvm/llvm-project/tree/master/polly";>Source @ GitHub
   
 
   
Index: polly/www/get_started.html
===
--- polly/www/get_started.html
+++ polly/www/get_started.html
@@ -33,20 +33,14 @@
  Manual 
  Get the code 
 
-Warning: Polly/LLVM/clang need to be checked out at the same time.
-
 
-git clone http://llvm.org/git/llvm.git llvm_git
-git clone http://llvm.org/git/polly.git llvm_git/tools/polly
-
-# Also build the matching clang-version (optional)
-git clone http://llvm.org/git/clang.git llvm_git/tools/clang
+git clone http://github.com/llvm/llvm-project.git llvm_git
 
 Build Polly
 
 
 mkdir llvm_build && cd llvm_build
-cmake ../llvm_git && make
+cmake -DLLVM_ENABLE_PROJECTS='polly;clang' ../llvm_git/llvm && make
 
 
  Test Polly
@@ -59,7 +53,7 @@
 build. To configure Polly to use a pre-built LLVM, set the
 -DCMAKE_PREFIX_PATH option:
 
-cmake -DCMAKE_PREFIX_PATH=${LLVM_PREFIX}/lib/cmake/llvm
+cmake -DCMAKE_PREFIX_PATH=${LLVM_PREFIX}/lib/cmake/llvm ../llvm_git/polly
 
 To run unittests, however, you need to have the LLVM source directory around.
 Polly will use the llvm-config of the LLVM you're building against
Index: polly/docs/TipsAndTricks.rst
===
--- polly/docs/TipsAndTricks.rst
+++ polly/docs/TipsAndTricks.rst
@@ -47,7 +47,7 @@
 the regression.
 
 LLVM has a single repository that contains all projects. It can be cloned at:
-``_. How to bisect on a
+``_. Ho

[PATCH] D53199: Fix the behavior of clang's -w flag.

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: clang/lib/Basic/DiagnosticIDs.cpp:460-463
+  // Honor -w: this disables all messages mapped to Warning severity, and 
*also*
+  // any diagnostics which are not Error/Fatal by default (that is, they were
+  // upgraded by any of the mechanisms available: -Werror, -pedantic, or 
#pragma
+  // diagnostic)

rsmith wrote:
> I think this would be clearer if phrased the other way around:
> 
> > [...] disables all messages that are not Error/Fatal by default, and also 
> > any diagnostics that are Error/Fatal by default but that have been 
> > downgraded to Warning severity by any of the mechanisms available: 
> > -Wno-error or #pragma diagnostic
Reworded.



Comment at: clang/lib/Basic/DiagnosticIDs.cpp:466
+if (Result == diag::Severity::Warning ||
+!isDefaultMappingAsError((diag::kind)DiagID))
+  return diag::Severity::Ignored;

rsmith wrote:
> I think this change will also cause `-w` to disable all remarks. Was that 
> your intent?
No, that seems like a bug.

Remarks have their own completely-separate set of command-line options; I don't 
think -R should interact with -w. I've added a conditional here, and a test 
case ensuring that.


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

https://reviews.llvm.org/D53199



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

[PATCH] D53199: Fix the behavior of clang's -w flag.

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 183966.
jyknight marked 2 inline comments as done.
jyknight added a comment.

Fix to not disable remarks, reword comment, adjust implementation-of-module.m 
test-case.


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

https://reviews.llvm.org/D53199

Files:
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/test/Frontend/optimization-remark.c
  clang/test/Frontend/warning-mapping-2.c
  clang/test/Frontend/warning-mapping-4.c
  clang/test/Frontend/warning-mapping-5.c
  clang/test/Frontend/warning-mapping-6.c
  clang/test/Modules/implementation-of-module.m

Index: clang/test/Modules/implementation-of-module.m
===
--- clang/test/Modules/implementation-of-module.m
+++ clang/test/Modules/implementation-of-module.m
@@ -1,17 +1,17 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -w -Werror=auto-import %s -I %S/Inputs \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Werror=auto-import %s -I %S/Inputs \
 // RUN: -fmodule-implementation-of category_right -fsyntax-only
 
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -w -Werror=auto-import %s -I %S/Inputs \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Werror=auto-import %s -I %S/Inputs \
 // RUN: -fmodule-implementation-of category_right -dM -E -o - 2>&1 | FileCheck %s
 // CHECK-NOT: __building_module
 
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -w -Werror=auto-import %s -I %S/Inputs \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Werror=auto-import %s -I %S/Inputs \
 // RUN: -fmodule-implementation-of category_left -verify
 
-// RUN: %clang_cc1 -x objective-c-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -w -Werror=auto-import %s -I %S/Inputs \
+// RUN: %clang_cc1 -x objective-c-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Werror=auto-import %s -I %S/Inputs \
 // RUN: -fmodule-implementation-of category_right -emit-pch -o %t.pch
-// RUN: %clang_cc1 -x objective-c-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -w -Werror=auto-import %s -I %S/Inputs \
+// RUN: %clang_cc1 -x objective-c-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Werror=auto-import %s -I %S/Inputs \
 // RUN: -DWITH_PREFIX -fmodules-ignore-macro=WITH_PREFIX -include-pch %t.pch -fmodule-implementation-of category_right
 
 #ifndef WITH_PREFIX
Index: clang/test/Frontend/warning-mapping-6.c
===
--- /dev/null
+++ clang/test/Frontend/warning-mapping-6.c
@@ -0,0 +1,9 @@
+// Check that "#pragma diagnostic error" is suppressed by -w.
+//
+// RUN: %clang_cc1 -verify -Werror -w %s
+
+// expected-no-diagnostics
+#pragma gcc diagnostic error "-Wsign-compare"
+int f0(int x, unsigned y) {
+  return x < y;
+}
Index: clang/test/Frontend/warning-mapping-5.c
===
--- clang/test/Frontend/warning-mapping-5.c
+++ clang/test/Frontend/warning-mapping-5.c
@@ -1,6 +1,5 @@
-// Check that #pragma diagnostic warning overrides -Werror. This matches GCC's
-// original documentation, but not its earlier implementations.
-// 
+// Check that #pragma diagnostic warning overrides -Werror.
+//
 // RUN: %clang_cc1 -verify -Werror %s
 
 #pragma clang diagnostic warning "-Wsign-compare"
Index: clang/test/Frontend/warning-mapping-4.c
===
--- clang/test/Frontend/warning-mapping-4.c
+++ clang/test/Frontend/warning-mapping-4.c
@@ -1,5 +1,9 @@
+// Verify that various combinations of flags properly keep the sign-compare
+// warning disabled.
+
 // RUN: %clang_cc1 -verify -Wno-error=sign-compare %s
 // RUN: %clang_cc1 -verify -Wsign-compare -w -Wno-error=sign-compare %s
+// RUN: %clang_cc1 -verify -w -Werror=sign-compare %s
 // expected-no-diagnostics
 
 int f0(int x, unsigned y) {
Index: clang/test/Frontend/warning-mapping-2.c
===
--- clang/test/Frontend/warning-mapping-2.c
+++ clang/test/Frontend/warning-mapping-2.c
@@ -1,5 +1,7 @@
-// Check that -w has lower priority than -pedantic-errors.
+// Check that -w takes precedence over -pedantic-errors.
 // RUN: %clang_cc1 -verify -pedantic-errors -w %s
 
-void f0() { f1(); } // expected-error {{implicit declaration of function}}
+// Expect *not* to see a diagnostic for "implicit declaration of function"
+// expected-no-diagnostics
 
+void f0() { f1(); }
Index: clang/test/Frontend/optimization-remark.c
===
--- clang/test/Frontend/optimization-remark.c
+++ clang/test/Frontend/optimization-remark.c
@@ -13,6 +13,9 @@
 // RUN: %clang_cc1 %s -Rpass=inline -Rno-everything -emit-llvm -

[PATCH] D57330: Adjust documentation for git migration.

[James Y Knight via Phabricator via cfe-commits]

jyknight marked 8 inline comments as done.
jyknight added a comment.

In D57330#1375096 , @labath wrote:

> I am not sure we should be recommending to people to place the build folder 
> under the llvm-project checkout. Is that how people use the monorepo build 
> nowadays? This is not an full out-of-source build, since the build folder is 
> still a subfolder of the repo root (and without a .gitignore file containing 
> the right build folder name, git will complain about untracked files in the 
> repository)...


Well, it's certainly how I do it. I find it the least confusing, because then I 
don't get mixed up as to which source tree a given build directory belongs to 
(or directories, as I may also have build-release, build-debug, etc).




Comment at: libcxx/docs/BuildingLibcxx.rst:57
   $ cd where-you-want-libcxx-to-live
-  $ # Check out llvm, libc++ and libc++abi.
-  $ ``svn co http://llvm.org/svn/llvm-project/llvm/trunk llvm``
-  $ ``svn co http://llvm.org/svn/llvm-project/libcxx/trunk libcxx``
-  $ ``svn co http://llvm.org/svn/llvm-project/libcxxabi/trunk libcxxabi``
+  $ # Check out the sources (includes everything, but we'll only use libcxx)
+  $ ``git clone https://github.com/llvm/llvm-project.git``

mehdi_amini wrote:
> Wonder if it is worth mentioning somewhere how to sparse-checkout?
Yes, I think it likely is worth mentioning that somewhere in the future -- but 
I'd rather not recommend it yet. There's two things that will impact that:

1. We'll need to decide where we plan to keep shared infrastructure (e.g. cmake 
macros etc) used by multiple subprojects within the repository.
2. It seems as though git is actually gaining new support for this kind of 
thing now, might be worth letting that mature a little bit.



Comment at: libcxxabi/www/index.html:86
   mkdir build && cd build
-  cmake .. # on linux you may need to prefix with CC=clang 
CXX=clang++
+  cmake -DLLVM_ENABLE_PROJECTS=libcxxabi ../llvm # on linux you may 
need to prefix with CC=clang CXX=clang++
   make

labath wrote:
> mehdi_amini wrote:
> > Do you now if prefixing with CC is equivalent to -DCMAKE_C_COMPILER?
> It usually is, but it can differ once you start using cache and toolchain 
> files. In any case, using -DCMAKE_C(XX)_COMPILER is the preferred way to do 
> things in cmake.
Changed to specify the cmake arguments.



Comment at: lld/docs/getting_started.rst:71
+ $ cd llvm-project/build (out of source build required)
+ $ cmake -G "Visual Studio 11" ../llvm
 

mehdi_amini wrote:
> Missing -DLLVM_ENABLE_PROJECTS=lld here I believe
Yep fixed.



Comment at: 
lldb/packages/Python/lldbsuite/test/functionalities/command_source/TestCommandSource.py:4
-
-See also http://llvm.org/viewvc/llvm-project?view=rev&revision=109673.
 """

smeenai wrote:
> Would you want to link to the corresponding GitHub commit?
After viewing said commit, I felt it wasn't actually useful to visit to 
understand this file, which is why I removed the link.



Comment at: 
lldb/packages/Python/lldbsuite/test/lang/objc/objc-optimized/TestObjcOptimized.py:4
 
-http://llvm.org/viewvc/llvm-project?rev=126973&view=rev
 Fixed a bug in the expression parser where the 'this'

smeenai wrote:
> Same here.
Felt the same in this case.


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

https://reviews.llvm.org/D57330



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

[PATCH] D57330: Adjust documentation for git migration.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
jyknight marked 2 inline comments as done.
Closed by commit rC352514: Adjust documentation for git migration. (authored by 
jyknight, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57330?vs=183891&id=184098#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57330

Files:
  .gitignore
  docs/ClangPlugins.rst
  docs/ClangTools.rst
  docs/ControlFlowIntegrityDesign.rst
  docs/InternalsManual.rst
  docs/LibASTMatchersTutorial.rst
  docs/LibTooling.rst
  docs/Toolchain.rst
  lib/CodeGen/CGOpenMPRuntime.cpp
  www/analyzer/checker_dev_manual.html
  www/get_started.html
  www/hacking.html
  www/menu.html.incl

Index: docs/LibTooling.rst
===
--- docs/LibTooling.rst
+++ docs/LibTooling.rst
@@ -196,6 +196,6 @@
 Linking
 ^^^
 
-For a list of libraries to link, look at one of the tools' Makefiles (for
-example `clang-check/Makefile
-`_).
+For a list of libraries to link, look at one of the tools' CMake files (for
+example `clang-check/CMakeList.txt
+`_).
Index: docs/InternalsManual.rst
===
--- docs/InternalsManual.rst
+++ docs/InternalsManual.rst
@@ -1686,7 +1686,7 @@
 ^^^
 The first step to adding a new attribute to Clang is to add its definition to
 `include/clang/Basic/Attr.td
-`_.
+`_.
 This tablegen definition must derive from the ``Attr`` (tablegen, not
 semantic) type, or one of its derivatives. Most attributes will derive from the
 ``InheritableAttr`` type, which specifies that the attribute can be inherited by
@@ -1748,10 +1748,10 @@
 either ``diag::warn_attribute_wrong_decl_type`` or
 ``diag::err_attribute_wrong_decl_type``, and the parameter enumeration is found
 in `include/clang/Sema/ParsedAttr.h
-`_
+`_
 If a previously unused Decl node is added to the ``SubjectList``, the logic used
 to automatically determine the diagnostic parameter in `utils/TableGen/ClangAttrEmitter.cpp
-`_
+`_
 may need to be updated.
 
 By default, all subjects in the SubjectList must either be a Decl node defined
@@ -1773,7 +1773,7 @@
 Documentation is table generated on the public web server by a server-side
 process that runs daily. Generally, the documentation for an attribute is a
 stand-alone definition in `include/clang/Basic/AttrDocs.td 
-`_
+`_
 that is named after the attribute being documented.
 
 If the attribute is not for public consumption, or is an implicitly-created
@@ -1824,7 +1824,7 @@
 optional. The associated C++ type of the argument is determined by the argument
 definition type. If the existing argument types are insufficient, new types can
 be created, but it requires modifying `utils/TableGen/ClangAttrEmitter.cpp
-`_
+`_
 to properly support the type.
 
 Other Properties
@@ -1836,7 +1836,7 @@
 If the parsed form of the attribute is more complex, or differs from the
 semantic form, the ``HasCustomParsing`` bit can be set to ``1`` for the class,
 and the parsing code in `Parser::ParseGNUAttributeArgs()
-`_
+`_
 can be updated for the special case. Note that this only applies to arguments
 with a GNU spelling -- attributes with a __declspec spelling currently ignore
 this flag and are handled by ``Parser::ParseMicrosoftDeclSpec``.
@@ -1899,7 +1899,7 @@
 Boilerplate
 ^^^
 All semantic processing of declaration attributes happens in `lib/Sema/SemaDeclAttr.cpp
-`_,
+`_,
 and generally starts in t

[PATCH] D53199: Fix the behavior of clang's -w flag.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352535: Fix the behavior of clang's -w flag. (authored 
by jyknight, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53199?vs=183966&id=184142#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D53199

Files:
  cfe/trunk/lib/Basic/DiagnosticIDs.cpp
  cfe/trunk/test/Frontend/optimization-remark.c
  cfe/trunk/test/Frontend/warning-mapping-2.c
  cfe/trunk/test/Frontend/warning-mapping-4.c
  cfe/trunk/test/Frontend/warning-mapping-5.c
  cfe/trunk/test/Frontend/warning-mapping-6.c
  cfe/trunk/test/Modules/implementation-of-module.m

Index: cfe/trunk/test/Frontend/optimization-remark.c
===
--- cfe/trunk/test/Frontend/optimization-remark.c
+++ cfe/trunk/test/Frontend/optimization-remark.c
@@ -13,6 +13,9 @@
 // RUN: %clang_cc1 %s -Rpass=inline -Rno-everything -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-NO-REMARKS
 // RUN: %clang_cc1 %s -Rpass=inline -Rno-everything -Reverything -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-REMARKS
 //
+// Check that -w doesn't disable remarks.
+// RUN: %clang_cc1 %s -Rpass=inline -w -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-REMARKS
+//
 // FIXME: -Reverything should imply -Rpass=.*.
 // RUN: %clang_cc1 %s -Reverything -emit-llvm -o - 2>/dev/null | FileCheck %s --check-prefix=CHECK-NO-REMARKS
 //
Index: cfe/trunk/test/Frontend/warning-mapping-5.c
===
--- cfe/trunk/test/Frontend/warning-mapping-5.c
+++ cfe/trunk/test/Frontend/warning-mapping-5.c
@@ -1,6 +1,5 @@
-// Check that #pragma diagnostic warning overrides -Werror. This matches GCC's
-// original documentation, but not its earlier implementations.
-// 
+// Check that #pragma diagnostic warning overrides -Werror.
+//
 // RUN: %clang_cc1 -verify -Werror %s
 
 #pragma clang diagnostic warning "-Wsign-compare"
Index: cfe/trunk/test/Frontend/warning-mapping-6.c
===
--- cfe/trunk/test/Frontend/warning-mapping-6.c
+++ cfe/trunk/test/Frontend/warning-mapping-6.c
@@ -0,0 +1,9 @@
+// Check that "#pragma diagnostic error" is suppressed by -w.
+//
+// RUN: %clang_cc1 -verify -Werror -w %s
+
+// expected-no-diagnostics
+#pragma gcc diagnostic error "-Wsign-compare"
+int f0(int x, unsigned y) {
+  return x < y;
+}
Index: cfe/trunk/test/Frontend/warning-mapping-4.c
===
--- cfe/trunk/test/Frontend/warning-mapping-4.c
+++ cfe/trunk/test/Frontend/warning-mapping-4.c
@@ -1,5 +1,9 @@
+// Verify that various combinations of flags properly keep the sign-compare
+// warning disabled.
+
 // RUN: %clang_cc1 -verify -Wno-error=sign-compare %s
 // RUN: %clang_cc1 -verify -Wsign-compare -w -Wno-error=sign-compare %s
+// RUN: %clang_cc1 -verify -w -Werror=sign-compare %s
 // expected-no-diagnostics
 
 int f0(int x, unsigned y) {
Index: cfe/trunk/test/Frontend/warning-mapping-2.c
===
--- cfe/trunk/test/Frontend/warning-mapping-2.c
+++ cfe/trunk/test/Frontend/warning-mapping-2.c
@@ -1,5 +1,7 @@
-// Check that -w has lower priority than -pedantic-errors.
+// Check that -w takes precedence over -pedantic-errors.
 // RUN: %clang_cc1 -verify -pedantic-errors -w %s
 
-void f0() { f1(); } // expected-error {{implicit declaration of function}}
+// Expect *not* to see a diagnostic for "implicit declaration of function"
+// expected-no-diagnostics
 
+void f0() { f1(); }
Index: cfe/trunk/test/Modules/implementation-of-module.m
===
--- cfe/trunk/test/Modules/implementation-of-module.m
+++ cfe/trunk/test/Modules/implementation-of-module.m
@@ -1,17 +1,17 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -w -Werror=auto-import %s -I %S/Inputs \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Werror=auto-import %s -I %S/Inputs \
 // RUN: -fmodule-implementation-of category_right -fsyntax-only
 
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -w -Werror=auto-import %s -I %S/Inputs \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Werror=auto-import %s -I %S/Inputs \
 // RUN: -fmodule-implementation-of category_right -dM -E -o - 2>&1 | FileCheck %s
 // CHECK-NOT: __building_module
 
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -w -Werror=auto-import %s -I %S/Inputs \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -Werror=auto-import %s -I %S/Inputs \
 // RUN: -fmodule-implementation-of category_left -verify
 
-// RUN: %clang_cc1 -x

[PATCH] D57315: [opaque pointer types] Add a FunctionCallee wrapper type, and use it.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352791: [opaque pointer types] Add a FunctionCallee wrapper 
type, and use it. (authored by jyknight, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57315?vs=183795&id=184576#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57315

Files:
  lib/CodeGen/CGExpr.cpp


Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3056,7 +3056,7 @@
   bool WithDiag = !CGM.getCodeGenOpts().SanitizeTrap.has(Kind);
 
   llvm::CallInst *CheckCall;
-  llvm::Constant *SlowPathFn;
+  llvm::FunctionCallee SlowPathFn;
   if (WithDiag) {
 llvm::Constant *Info = llvm::ConstantStruct::getAnon(StaticArgs);
 auto *InfoPtr =
@@ -3078,7 +3078,8 @@
 CheckCall = Builder.CreateCall(SlowPathFn, {TypeId, Ptr});
   }
 
-  CGM.setDSOLocal(cast(SlowPathFn->stripPointerCasts()));
+  CGM.setDSOLocal(
+  cast(SlowPathFn.getCallee()->stripPointerCasts()));
   CheckCall->setDoesNotThrow();
 
   EmitBlock(Cont);


Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3056,7 +3056,7 @@
   bool WithDiag = !CGM.getCodeGenOpts().SanitizeTrap.has(Kind);
 
   llvm::CallInst *CheckCall;
-  llvm::Constant *SlowPathFn;
+  llvm::FunctionCallee SlowPathFn;
   if (WithDiag) {
 llvm::Constant *Info = llvm::ConstantStruct::getAnon(StaticArgs);
 auto *InfoPtr =
@@ -3078,7 +3078,8 @@
 CheckCall = Builder.CreateCall(SlowPathFn, {TypeId, Ptr});
   }
 
-  CGM.setDSOLocal(cast(SlowPathFn->stripPointerCasts()));
+  CGM.setDSOLocal(
+  cast(SlowPathFn.getCallee()->stripPointerCasts()));
   CheckCall->setDoesNotThrow();
 
   EmitBlock(Cont);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34294: Rework libcxx strerror_r handling.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL308528: Rework libcxx strerror_r handling. (authored by 
jyknight).

Repository:
  rL LLVM

https://reviews.llvm.org/D34294

Files:
  libcxx/trunk/src/system_error.cpp


Index: libcxx/trunk/src/system_error.cpp
===
--- libcxx/trunk/src/system_error.cpp
+++ libcxx/trunk/src/system_error.cpp
@@ -73,39 +73,59 @@
   std::snprintf(buffer, strerror_buff_size, "unknown error %d", ev);
   return string(buffer);
 }
-#elif defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC) && 
\
-(!defined(__ANDROID__) || __ANDROID_API__ >= 23)
-// GNU Extended version
-string do_strerror_r(int ev) {
-char buffer[strerror_buff_size];
-char* ret = ::strerror_r(ev, buffer, strerror_buff_size);
-return string(ret);
-}
 #else
-// POSIX version
+
+// Only one of the two following functions will be used, depending on
+// the return type of strerror_r:
+
+// For the GNU variant, a char* return value:
+__attribute__((unused)) const char *
+handle_strerror_r_return(char *strerror_return, char *buffer) {
+  // GNU always returns a string pointer in its return value. The
+  // string might point to either the input buffer, or a static
+  // buffer, but we don't care which.
+  return strerror_return;
+}
+
+// For the POSIX variant: an int return value.
+__attribute__((unused)) const char *
+handle_strerror_r_return(int strerror_return, char *buffer) {
+  // The POSIX variant either:
+  // - fills in the provided buffer and returns 0
+  // - returns a positive error value, or
+  // - returns -1 and fills in errno with an error value.
+  if (strerror_return == 0)
+return buffer;
+
+  // Only handle EINVAL. Other errors abort.
+  int new_errno = strerror_return == -1 ? errno : strerror_return;
+  if (new_errno == EINVAL)
+return "";
+
+  _LIBCPP_ASSERT(new_errno == ERANGE, "unexpected error from ::strerror_r");
+  // FIXME maybe? 'strerror_buff_size' is likely to exceed the
+  // maximum error size so ERANGE shouldn't be returned.
+  std::abort();
+}
+
+// This function handles both GNU and POSIX variants, dispatching to
+// one of the two above functions.
 string do_strerror_r(int ev) {
 char buffer[strerror_buff_size];
+// Preserve errno around the call. (The C++ standard requires that
+// system_error functions not modify errno).
 const int old_errno = errno;
-int ret;
-if ((ret = ::strerror_r(ev, buffer, strerror_buff_size)) != 0) {
-// If `ret == -1` then the error is specified using `errno`, otherwise
-// `ret` represents the error.
-const int new_errno = ret == -1 ? errno : ret;
-errno = old_errno;
-if (new_errno == EINVAL) {
-std::snprintf(buffer, strerror_buff_size, "Unknown error %d", ev);
-return string(buffer);
-} else {
-_LIBCPP_ASSERT(new_errno == ERANGE, "unexpected error from 
::strerr_r");
-// FIXME maybe? 'strerror_buff_size' is likely to exceed the
-// maximum error size so ERANGE shouldn't be returned.
-std::abort();
-}
+const char *error_message = handle_strerror_r_return(
+::strerror_r(ev, buffer, strerror_buff_size), buffer);
+// If we didn't get any message, print one now.
+if (!error_message[0]) {
+  std::snprintf(buffer, strerror_buff_size, "Unknown error %d", ev);
+  error_message = buffer;
 }
-return string(buffer);
+errno = old_errno;
+return string(error_message);
 }
 #endif
-
 } // end namespace
 #endif
 


Index: libcxx/trunk/src/system_error.cpp
===
--- libcxx/trunk/src/system_error.cpp
+++ libcxx/trunk/src/system_error.cpp
@@ -73,39 +73,59 @@
   std::snprintf(buffer, strerror_buff_size, "unknown error %d", ev);
   return string(buffer);
 }
-#elif defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC) && \
-(!defined(__ANDROID__) || __ANDROID_API__ >= 23)
-// GNU Extended version
-string do_strerror_r(int ev) {
-char buffer[strerror_buff_size];
-char* ret = ::strerror_r(ev, buffer, strerror_buff_size);
-return string(ret);
-}
 #else
-// POSIX version
+
+// Only one of the two following functions will be used, depending on
+// the return type of strerror_r:
+
+// For the GNU variant, a char* return value:
+__attribute__((unused)) const char *
+handle_strerror_r_return(char *strerror_return, char *buffer) {
+  // GNU always returns a string pointer in its return value. The
+  // string might point to either the input buffer, or a static
+  // buffer, but we don't care which.
+  return strerror_return;
+}
+
+// For the POSIX variant: an int return value.
+__attribute__((unused)) const char *
+handle_strerror_r_return(int strerror_return, char *buffer) {
+  // The POSIX variant either:
+  // - fills in the provided buffer and returns 0
+

[PATCH] D34158: For standards compatibility, preinclude if the file is available

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote:

> Hmm... I tried this patch and now the following worries me:
>
> - it passes -finclude-if-exists stdc-predef.h on all platforms (say, 
> including my Solaris platform that has no system stdc-predef.h)


Right, but Solaris probably _ought_ to add one as well, to define those macros.

> - it searches all the paths, not just "system include" ones
> 
>   That essentially disallows user to have stdc-predef.h include in my own 
> project, since there is a chance that this user header will be accidentally 
> included by this hidden machinery.

IMO, this is a fairly negligible issue, and so we go *shrug* oh well.

+1 for using a <> include -- that does seem better.

But, note, that will have no effect w.r.t. this issue for most users, since 
typically people use "cc -Isomepath", which adds 'somepath' to the list which 
gets searched by both <> and "" includes. Hardly anyone uses -iquote.


Repository:
  rL LLVM

https://reviews.llvm.org/D34158



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

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In https://reviews.llvm.org/D34158#827178, @joerg wrote:

> I had a long discussion with James about this on IRC without reaching a clear 
> consensus. I consider forcing this behavior on all targets to be a major bug. 
> It should be opt-in and opt-in only:
>
> (1) The header name is not mandated by any standard. It is not in any 
> namespace generally accepted as implementation-owned.


This is a point. I didn't think it was a big deal, but if you want to argue a 
different name should be used, that's a reasonable argument. If we can get some 
agreement amongst other libc vendors to use some more agreeable alternative 
name, and keep a fallback on linux-only for the "stdc-predef.h" name, I'd 
consider that as a great success.

> (2) It adds magic behavior that can make debugging more difficult. Partially 
> preprocessed sources for example could be compiled with plain -c before, now 
> they need a different command line.

If this is a problem, making it be Linux-only does _nothing_ to solve it. But I 
don't actually see how this is a substantively new problem? Compiling with 
plain -c before would get #defines for those predefined macros that the 
compiler sets, even though you may not have wanted those. Is this fundamentally 
different?

> (3) It seems to me that the GNU userland (and maybe Windows) is the exception 
> to a well integrated tool chain. Most other platforms have a single canonical 
> libc, libm and libpthread implementation and can as such directly define all 
> the relevant macros directly in the driver.

I don't think this is accurate. There's many platforms out there, and for 
almost none of them do we have exact knowledge of the features of the libc 
encoded into the compiler. I'd note that not only do you need this for every 
(OS, libc) combination, you'd need it for every (OS, libc-VERSION) combination.

> Given that many of the macros involved are already reflected by the compiler 
> behavior anyway, they can't be decoupled. I.e. the questionable concept of 
> locale-independent wchar_t is already hard-coded in the front end as soon as 
> any long character literals are used.

AFAICT, this example is not actually the case. The frontend only needs to know 
*a* valid encoding for wide-character literals in some implementation-defined 
locale (for example, it might always emit them as unicode codepoints, as clang 
does).  Standard says: "the array elements [...] are initialized with the 
sequence of wide characters corresponding to the multibyte character sequence, 
as defined by the mbstowcs function with an implementation defined current 
locale."

On the other hand, I believe the intent of this macro is to guarantee that _no 
matter what_ the locale is, that a U+0100 character (say, translated with 
mbrtowc from the locale encoding) gets represented as 0x100.

Thus, it's "fine" for the frontend to always emit wchar_t literals as unicode, 
yet, the libc may sometimes use an arbitrary different internal encoding, 
depending on the locale used at runtime. (Obviously using wide character 
literals with such a libc would be a poor user experience, and such a libc 
probably ought to reconsider its choices, but that's a different discussion.)

> As such, please move the command line additions back into the target-specific 
> files for targets that actually want to get this behavior.

Without even a suggestion of a better solution to use for other targets, I find 
it is to be a real shame to push for this to be linux-only, and leave everyone 
else hanging. I find that that _most_ of these defines are effectively library 
decisions. I further would claim that these are likely to vary over the 
lifetime of even a single libc, and that as such we would be better served by 
allowing the libc to set them directly, rather than encoding it into the 
compiler.

TTBOMK, no targets except linux/glibc/gcc actually comply with this part of the 
C99/C11 standard today, and so this feature would be useful to have available 
across all targets.

(I very much dislike that the C standard has started adding all these new 
predefined macros, instead of exposing them from a header, but there's not much 
to be done about that...)

Going through the various macros:
`__STDC_ISO_10646__`:
As discussed above, this is effectively entirely up to the libc. The compiler 
only need support one possible encoding for wchar_t, and clang always chooses 
unicode. But it can't define this because the libc might use others as well.

`__STDC_MB_MIGHT_NEQ_WC__`:
As with `__STDC_ISO_10646__`, this is up to the libc not the compiler. (At 
least, I think so... I note that clang currently sets this for freebsd, with a 
FIXME next to it saying it's only intended to apply to wide character literals. 
I don't see that the standard says that, however, so, IMO, having it set for 
freebsd was and is correct).

`__STDC_UTF16__`, `__STDC_UTF32__`:
Again, analogous to `__STDC_ISO_10646__`, except dealing with 
cha

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Just to restate: the ideal outcome of this discussion for me would be to 
resolve things such that _ALL_ libc implementations will feel comfortable using 
this technique to provide the C11-required predefined macros.

I'd love for linux, freebsd, macos, solaris, etc etc libc to all conform to the 
C standard in this regards, and do so in a common way, without the need to 
encode information about each libc version into the compiler. I _really_ don't 
think that scales well.

So I take your comments from FreeBSD's point of view seriously, and would very 
much like to understand and hopefully resolve them.

In https://reviews.llvm.org/D34158#837130, @joerg wrote:

> In https://reviews.llvm.org/D34158#836026, @jyknight wrote:
>
> > In https://reviews.llvm.org/D34158#827178, @joerg wrote:
> >
> > > (2) It adds magic behavior that can make debugging more difficult. 
> > > Partially preprocessed sources for example could be compiled with plain 
> > > -c before, now they need a different command line.
> >
> >
> > If this is a problem, making it be Linux-only does _nothing_ to solve it. 
> > But I don't actually see how this is a substantively new problem? Compiling 
> > with plain -c before
> >  would get #defines for those predefined macros that the compiler sets, 
> > even though you may not have wanted those. Is this fundamentally different?
>
>
> It makes it a linux-only problem. As such, it is something *I* only care 
> about secondary. A typical use case I care about a lot is pulling the crash 
> report sources from my (NetBSD) build machine,
>  extracting the original command line to rerun the normal compilation with 
> -save-temps. I don't necessarily have the (same) system headers on the 
> machine I use for debugging and that's exactly
>  the kind of use case this change breaks. All other predefined macros are 
> driven by the target triple and remain stable.


"it's Linux only so I don't care if it's broken." is still not very helpful. :)

But I do think understand what you're saying now, so thanks for the elaboration.

Firstly, let's consider a "clang foo.i" or "clang -x cpp-output foo.c" 
compilation. In that case, it *clearly* should not be including the predef 
file. I think the patch as it stands may not do this properly. A test needs to 
be added for this to this patch, and perhaps the behavior needs to be fixed as 
well.

(Sidenote: clang doesn't support preprocessed input properly, but that's 
another bug, and we certainly ought not make it worse. Check out e.g. "int 
main() { return __GNUC__; }". it should report that __GNUC__ is undeclared, but 
instead compiles a program that returns 4.)

But, that's not the case you're talking about above -- you're not talking about 
compiling preprocessed output, you're talking about taking output that comes 
from -frewrite-includes.

Let me recap the scenario:

1. Start with a source file foo.c, with this content:

#include 
#pragma clang __debug parser_crash

2. Run "clang foo.c". It crashes, and dumps a /tmp/foo-XXX.c and a 
/tmp/foo-XXX.sh script.

The .c file is generated via -frewrite-includes, so it's _not_ already 
preprocessed, it simply has all includes pulled into a single file. It also 
_doesn't_ insert the compiler-predefined macros at the top, but it _will_ 
include the content of this stdc-predef.h file.

OK, so then...
The generated script invokes a -cc1 command line, with all the include 
arguments stripped out of the command. (TO FIX: We should be stripping the new 
arg as well: add "-fsystem-include-if-exists" argument to the list of include 
things in the skipArgs() function in lib/Driver/Job.cpp). Even without that 
change, it's actually already fine, as there is no include path specified in 
which to find the file -- but it's cleaner to strip it, so let's do that. The 
reproducer script will thus run correctly, and not include the file.

Now, the "/tmp/foo-XXX.sh" also has a line labeled "Driver args: " with the 
original command-line on it. If I understand correctly, you then like to take 
this simpler Driver command-line, and edit it manually: add -save-temps, and 
change the input filename  to the "/tmp/foo-XXX.c" file, and run that, instead 
of actually invoking the reproducer foo-XXX.sh.

Since stdc-predef.h is included automatically, it will now be present twice -- 
first, it will read the one from your system's /usr/include, and then the copy 
inlined into the /tmp/foo-XXX.c file. That's not what you desired. You wanted 
nothing from your /usr/include to be used.

The fix for the end-user here is easy: you can add -nostdinc which will 
suppress all the default include paths, and thus it will not find this predef 
file from your system include dir.

I'll note that you'd also have had an issue if the original driver command-line 
had a "-include" option in it, which you would have needed to edit out manually 
as well. (But I understand that is less common.)

Have I correctly described the situation? I g

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In https://reviews.llvm.org/D34158#838454, @mibintc wrote:

> This patch is responding to @jyknight 's concern about preprocessed input. 
> The patch as it stands doesn't have this issue. I added 2 test cases, one 
> using option -x cpp-output, and another for a source file suffixed with .i
>
> Quoting James: "Firstly, let's consider a "clang foo.i" or "clang -x 
> cpp-output foo.c" compilation. In that case, it *clearly* should not be 
> including the predef file. I think the patch as it stands may not do this 
> properly. A test needs to be added for this to this patch, and perhaps the 
> behavior needs to be fixed as well."


Thanks for the test. It wasn't obvious from the code, so I'm glad to hear it 
was already correct. :)

Did you see the other suggestion I cleverly hid within a big block of 
commentary? "(TO FIX: We should be stripping the new arg as well: add 
"-fsystem-include-if-exists" argument to the list of include things in the 
skipArgs() function in lib/Driver/Job.cpp)"


https://reviews.llvm.org/D34158



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

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

That GCC and Clang differ in handling of Atomics is a really unfortunate, 
longstanding issue.

See https://bugs.llvm.org/show_bug.cgi?id=26462

For RISCV, perhaps it's not yet too late to have the RISCV psABI actually 
specify a single ABI for C11 _Atomic which all compilers can implement, rather 
than having compilers do whatever they want to, separately...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57450



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

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D57450#1641190 , @lenary wrote:

> @jyknight I hear where you're coming from. I'll see what I can do about the 
> psABI document.
>
> In that ticket, it's mentioned that the Darwin ABI explicitly says that 
> non-power-of-two atomic types should be padded and realigned, but I cannot 
> find any documentation explaining this. That would be useful, given 
> presumably GCC does have to pad/align on Darwin.


AFAIK, there is no such documentation, at least publicly. Possibly Apple has 
some internally, but I suspect it more likely just some in-person conversation 
or something.

GCC is not really supported on Darwin, so I suspect it just gets it wrong.

> Then the only outstanding question relates to zero-sized atomics, which GCC 
> does not pad, but I think Clang has to pad to get the semantics correct, 
> based on this comment: 
> https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ASTContext.cpp#L2176

The semantics in GCC are that you can create such an object, but any attempt to 
load or store it will result in a compile-time error. E.g., "error: argument 1 
of ‘__atomic_load’ must be a pointer to a nonzero size object". So I don't 
think there's really an issue there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57450



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

[PATCH] D66822: Hardware cache line size builtins

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

numbers for cacheline size.

In D66822#1647664 , @zoecarver wrote:

> > Passing-by remark: i'm not sure it is possible to **guarantee** that this 
> > will be always correct and that no ABI break will happen.
>
> You're right. I should have said, "least-likely to cause an ABI break." And I 
> completely agree that there is **no way** to gaurentee this is correct at 
> compile time. `hardware_*_interference_size` certainly has the potential to 
> do more harm than good but, I think that is another discussion.


I don't see why we'd bother to implement this as a builtin, if we're going to 
implement it like this. A much simpler implementation would be to have libc++ 
return 64 for constructive and 128 for destructive, across the board. That'd 
certainly be abi stable, and also correct, at the moment, for architectures 
people generally care about. (And we should tell people to never use these if 
they actually care about it.)

BTW, I note that facebook uses 128 bytes for x86, noting in the source 
:

  Microbenchmarks indicate that pairs of cache lines also see destructive
  interference under heavy use of atomic operations, as observed for atomic
  increment on Sandy Bridge.
  
  We assume a cache line size of 64, so we use a cache line pair size of 128
  to avoid destructive interference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66822



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

[PATCH] D64487: [clang, test] Fix Clang :: Headers/max_align.c on 64-bit SPARC

[James Y Knight via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64487



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

[PATCH] D64793: [Driver] Properly use values-X[ca].o, values-xpg[46].o on Solaris

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Is this really necessary? Users don't typically pass -std= to the driver for 
linking anyways (what do you even pass if you've compiled both C and C++ code?) 
so this seems a rather odd way to control behavior.

How about instead just adding "values-xpg6.o" unconditionally, alongside the 
current unconditional "values-Xa.o", and just forget about the xpg4 and Xc 
modes?




Comment at: lib/Driver/ToolChains/Solaris.cpp:16
 #include "clang/Driver/Options.h"
+#include "clang/Frontend/LangStandard.h"
 #include "llvm/Option/ArgList.h"

I'm not sure that's an acceptable dependency -- I think Driver probably is not 
supposed to depend on Frontend. If so, I guess LangStandard should move to 
clang/Basic/. Which also means moving InputKind from 
clang/include/clang/Frontend/FrontendOptions.h.

(Maybe someone else can weigh in on this question?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D64793



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

[PATCH] D64793: [Driver] Properly use values-X[ca].o, values-xpg[46].o on Solaris

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

> I fear it is necessary: at least it matches documented behaviour of both the 
> Sun/Oracle Studio compilers and gcc.

I will defer to your opinion here. But -- one last attempt at dissuading you. :)

Is this really doing something _important_, or is it just legacy cruft which 
can be safely ignored by now? With your "logb" example, it seems to me that it 
is probably preferable to always use the new correct "xpg6" implementation, and 
just ignore the legacy/incorrect version. Similarly, the example given in 
https://gcc.gnu.org/PR40411 of freopen -- again, seems like it'd be better to 
just use the new xpg6 behavior, always.

> The -std= options usually get passed to the linking step because CFLAGS is 
> added to the options as well

With gnu make they are not (unless it's doing a single-step compiling+linking 
step). Other tools like CMake also don't pass standards versions to linking. 
This makes sense, because a program can contain code compiled with multiple 
standards versions, and multiple languages. Thus, I'd expect most users to just 
get the default xpg6 and Xa objects, even if they do specify -std=c90 for 
compilation.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64793



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

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I'm not sure the history behind why these were added as default-on 
warningsthey don't really seem appropriate as default warnings to me, 
either.

But I think someone else probably ought to approve the change.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65192



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

[PATCH] D65582: IR: accept and print numbered %N names for function args

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.
Herald added a subscriber: wuzish.

+1 for doing this. I started looking at fixing this when I modified the printer 
to print proper labels for numbered basic-blocks (instead of comments), but I 
didn't do so because of the amount of test churn was off-putting.

I think that after this change, there's only one local entity left which uses a 
local-value-number but doesn't print it: the entry block. That also would cause 
a quite-large amount of test-churn to add.




Comment at: llvm/lib/IR/AsmWriter.cpp:3561
+else
+  Out << "";
   }

I think you need a space before this string? Although, then shouldn't 
llvm/unittests/IR/AsmWriterTest.cpp be failing? (it has a space there...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65582



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

[PATCH] D67573: Fix __atomic_is_lock_free's return type.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: rsmith.
Herald added subscribers: cfe-commits, jfb, kristof.beyls.
Herald added a project: clang.

It is specified to return a bool in GCC's documentation and
implementation, but the clang builtin says it returns an int. This
would be pretty much harmless, if it was just the builtin.

However, when clang translates the builtin into a libcall, it _also_
generates the libcall with an int return type, while the actual
library function in libatomic returns a bool.

This mismatch in return types results in an actual ABI mismatch and
thus incorrect results (interpreting false return value as true) on at
least x86_64.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67573

Files:
  clang/include/clang/Basic/Builtins.def
  clang/test/CodeGen/atomic-ops.c
  clang/test/CodeGen/big-atomic-ops.c


Index: clang/test/CodeGen/big-atomic-ops.c
===
--- clang/test/CodeGen/big-atomic-ops.c
+++ clang/test/CodeGen/big-atomic-ops.c
@@ -198,20 +198,20 @@
 int lock_free(struct Incomplete *incomplete) {
   // CHECK: @lock_free
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 3, i8* null)
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 3, i8* null)
   __c11_atomic_is_lock_free(3);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 16, i8* {{.*}}@sixteen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 16, i8* 
{{.*}}@sixteen{{.*}})
   __atomic_is_lock_free(16, &sixteen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 17, i8* {{.*}}@seventeen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 17, i8* 
{{.*}}@seventeen{{.*}})
   __atomic_is_lock_free(17, &seventeen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 4, {{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 4, {{.*}})
   __atomic_is_lock_free(4, incomplete);
 
   char cs[20];
-  // CHECK: call i32 @__atomic_is_lock_free(i64 4, {{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 4, {{.*}})
   __atomic_is_lock_free(4, cs+1);
 
   // CHECK-NOT: call
Index: clang/test/CodeGen/atomic-ops.c
===
--- clang/test/CodeGen/atomic-ops.c
+++ clang/test/CodeGen/atomic-ops.c
@@ -343,20 +343,20 @@
 int lock_free(struct Incomplete *incomplete) {
   // CHECK-LABEL: @lock_free
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 3, i8* null)
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 3, i8* null)
   __c11_atomic_is_lock_free(3);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 16, i8* {{.*}}@sixteen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 16, i8* 
{{.*}}@sixteen{{.*}})
   __atomic_is_lock_free(16, &sixteen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 17, i8* {{.*}}@seventeen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 17, i8* 
{{.*}}@seventeen{{.*}})
   __atomic_is_lock_free(17, &seventeen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 4, {{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 4, {{.*}})
   __atomic_is_lock_free(4, incomplete);
 
   char cs[20];
-  // CHECK: call i32 @__atomic_is_lock_free(i32 4, {{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 4, {{.*}})
   __atomic_is_lock_free(4, cs+1);
 
   // CHECK-NOT: call
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -720,7 +720,7 @@
 ATOMIC_BUILTIN(__c11_atomic_fetch_xor, "v.", "t")
 BUILTIN(__c11_atomic_thread_fence, "vi", "n")
 BUILTIN(__c11_atomic_signal_fence, "vi", "n")
-BUILTIN(__c11_atomic_is_lock_free, "iz", "n")
+BUILTIN(__c11_atomic_is_lock_free, "bz", "n")
 
 // GNU atomic builtins.
 ATOMIC_BUILTIN(__atomic_load, "v.", "t")
@@ -748,7 +748,7 @@
 BUILTIN(__atomic_thread_fence, "vi", "n")
 BUILTIN(__atomic_signal_fence, "vi", "n")
 BUILTIN(__atomic_always_lock_free, "izvCD*", "n")
-BUILTIN(__atomic_is_lock_free, "izvCD*", "n")
+BUILTIN(__atomic_is_lock_free, "bzvCD*", "n")
 
 // OpenCL 2.0 atomic builtins.
 ATOMIC_BUILTIN(__opencl_atomic_init, "v.", "t")


Index: clang/test/CodeGen/big-atomic-ops.c
===
--- clang/test/CodeGen/big-atomic-ops.c
+++ clang/test/CodeGen/big-atomic-ops.c
@@ -198,20 +198,20 @@
 int lock_free(struct Incomplete *incomplete) {
   // CHECK: @lock_free
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 3, i8* null)
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 3, i8* null)
   __c11_atomic_is_lock_free(3);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 16, i8* {{.*}}@sixteen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 16, i8* {{.*}}@sixteen{{.*}})
   __atomic_is_lock_free(16, &sixteen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 17, i8* {{.*}}@seventeen{{.*}})
+  // CHECK: call zeroext i1 @__

[PATCH] D67982: [ObjC] Add some additional test cases around pointer conversions.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: rjmccall.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is especially important for Objective-C++, which is entirely
missing this testing at the moment.

This annotates with "FIXME" the cases which I change in the next
patch -- I primarily wanted to document the current state of things so
that the effect of the code change is made clear.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67982

Files:
  clang/test/SemaObjC/class-method-self.m
  clang/test/SemaObjC/comptypes-1.m
  clang/test/SemaObjC/comptypes-7.m
  clang/test/SemaObjCXX/class-method-self.mm
  clang/test/SemaObjCXX/comptypes-1.mm
  clang/test/SemaObjCXX/comptypes-7.mm
  clang/test/SemaObjCXX/instancetype.mm

Index: clang/test/SemaObjCXX/instancetype.mm
===
--- clang/test/SemaObjCXX/instancetype.mm
+++ clang/test/SemaObjCXX/instancetype.mm
@@ -5,7 +5,7 @@
 #endif
 
 @interface Root
-+ (instancetype)alloc;
++ (instancetype)alloc; // FIXME -- should note {{explicitly declared 'instancetype'}}
 - (instancetype)init; // expected-note{{overridden method is part of the 'init' method family}}
 - (instancetype)self; // expected-note {{explicitly declared 'instancetype'}}
 - (Class)class;
@@ -143,7 +143,7 @@
 
 @implementation Subclass4
 + (id)alloc {
-  return self; // FIXME: we accept this in ObjC++ but not ObjC?
+  return self; // FIXME -- should error{{cannot initialize return object of type 'Subclass4 *' with an lvalue of type 'Class'}}
 }
 
 - (Subclass3 *)init { return 0; } // don't complain: we lost the related return type
Index: clang/test/SemaObjCXX/comptypes-7.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/comptypes-7.mm
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic %s
+
+#define nil nullptr
+
+extern void foo();
+
+@protocol MyProtocol
+- (void) method;
+@end
+
+@interface MyClass
+@end
+
+int main()
+{
+  id obj = nil;
+  id  obj_p = nil;
+  MyClass *obj_c = nil;
+  Class obj_C = nil;
+  
+  int i = 0;
+  int *j = nil;
+
+  /* These should all generate errors.  */
+  
+  obj = i; // expected-error {{assigning to 'id' from incompatible type 'int'}}
+  obj = j; // expected-error {{assigning to 'id' from incompatible type 'int *'}}
+
+  obj_p = i; // expected-error {{assigning to 'id' from incompatible type 'int'}}
+  obj_p = j; // expected-error {{assigning to 'id' from incompatible type 'int *'}}
+  
+  obj_c = i; // expected-error {{assigning to 'MyClass *' from incompatible type 'int'}}
+  obj_c = j; // expected-error {{assigning to 'MyClass *' from incompatible type 'int *'}}
+
+  obj_C = i; // expected-error {{assigning to 'Class' from incompatible type 'int'}}
+  obj_C = j; // expected-error {{assigning to 'Class' from incompatible type 'int *'}}
+  
+  i = obj;   // expected-error {{assigning to 'int' from incompatible type 'id'}}
+  i = obj_p; // expected-error {{assigning to 'int' from incompatible type 'id'}}
+  i = obj_c; // expected-error {{assigning to 'int' from incompatible type 'MyClass *'}}
+  i = obj_C; // expected-error {{assigning to 'int' from incompatible type 'Class'}}
+  
+  j = obj;   // expected-error {{assigning to 'int *' from incompatible type 'id'}}
+  j = obj_p; // expected-error {{assigning to 'int *' from incompatible type 'id'}}
+  j = obj_c; // expected-error {{assigning to 'int *' from incompatible type 'MyClass *'}}
+  j = obj_C; // expected-error {{assigning to 'int *' from incompatible type 'Class'}}
+
+  if (obj == i) foo() ; // expected-error {{comparison between pointer and integer ('id' and 'int')}}
+  if (i == obj) foo() ; // expected-error {{comparison between pointer and integer ('int' and 'id')}}
+  if (obj == j) foo() ; // expected-error {{invalid operands to binary expression ('id' and 'int *')}}
+  if (j == obj) foo() ; // expected-error {{invalid operands to binary expression ('int *' and 'id')}}
+
+  if (obj_c == i) foo() ; // expected-error {{comparison between pointer and integer ('MyClass *' and 'int')}}
+  if (i == obj_c) foo() ; // expected-error {{comparison between pointer and integer ('int' and 'MyClass *')}}
+  if (obj_c == j) foo() ; // expected-error {{invalid operands to binary expression ('MyClass *' and 'int *')}}
+  if (j == obj_c) foo() ; // expected-error {{invalid operands to binary expression ('int *' and 'MyClass *')}}
+
+  if (obj_p == i) foo() ; // expected-error {{comparison between pointer and integer ('id' and 'int')}}
+  if (i == obj_p) foo() ; // expected-error {{comparison between pointer and integer ('int' and 'id')}}
+  if (obj_p == j) foo() ; // expected-error {{invalid operands to binary expression ('id' and 'int *')}}
+  if (j == obj_p) foo() ; // expected-error {{invalid operands to binary expression ('int *' and 'id')}}
+
+  if (obj_C == i) foo() ; // expected-error {{comparison between pointer and int

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: rjmccall.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For example, in Objective-C mode, the initialization of 'x' in:

  @implementation MyType
  + (void)someClassMethod {
MyType *x = self;
  }
  @end

is correctly diagnosed with an incompatible-pointer-types warning, but
in Objective-C++ mode, it is not diagnosed at all -- even though
incompatible pointer conversions generally become an error in C++.

This patch fixes that oversight, allowing implicit conversions
involving Class only to/from unqualified-id, and between qualified and
unqualified Class, where the protocols are compatible.

Note that this does change some behaviors in Objective-C, as well, as
shown by the modified tests.

Of particular note is that assignment from from 'Class' to
'id' now warns. (Despite appearances, those are not
compatible types. 'Class' is not expected to have instance
methods defined by 'MyProtocol', while 'id' is.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67983

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaObjC/comptypes-1.m
  clang/test/SemaObjCXX/class-method-self.mm
  clang/test/SemaObjCXX/comptypes-1.mm
  clang/test/SemaObjCXX/comptypes-7.mm
  clang/test/SemaObjCXX/instancetype.mm

Index: clang/test/SemaObjCXX/instancetype.mm
===
--- clang/test/SemaObjCXX/instancetype.mm
+++ clang/test/SemaObjCXX/instancetype.mm
@@ -5,7 +5,7 @@
 #endif
 
 @interface Root
-+ (instancetype)alloc; // FIXME -- should note {{explicitly declared 'instancetype'}}
++ (instancetype)alloc; // expected-note {{explicitly declared 'instancetype'}}
 - (instancetype)init; // expected-note{{overridden method is part of the 'init' method family}}
 - (instancetype)self; // expected-note {{explicitly declared 'instancetype'}}
 - (Class)class;
@@ -143,7 +143,7 @@
 
 @implementation Subclass4
 + (id)alloc {
-  return self; // FIXME -- should error{{cannot initialize return object of type 'Subclass4 *' with an lvalue of type 'Class'}}
+  return self; // expected-error{{cannot initialize return object of type 'Subclass4 *' with an lvalue of type 'Class'}}
 }
 
 - (Subclass3 *)init { return 0; } // don't complain: we lost the related return type
Index: clang/test/SemaObjCXX/comptypes-7.mm
===
--- clang/test/SemaObjCXX/comptypes-7.mm
+++ clang/test/SemaObjCXX/comptypes-7.mm
@@ -47,23 +47,23 @@
 
   if (obj == i) foo() ; // expected-error {{comparison between pointer and integer ('id' and 'int')}}
   if (i == obj) foo() ; // expected-error {{comparison between pointer and integer ('int' and 'id')}}
-  if (obj == j) foo() ; // expected-error {{invalid operands to binary expression ('id' and 'int *')}}
-  if (j == obj) foo() ; // expected-error {{invalid operands to binary expression ('int *' and 'id')}}
+  if (obj == j) foo() ; // expected-error {{comparison of distinct pointer types ('id' and 'int *')}}
+  if (j == obj) foo() ; // expected-error {{comparison of distinct pointer types ('int *' and 'id')}}
 
   if (obj_c == i) foo() ; // expected-error {{comparison between pointer and integer ('MyClass *' and 'int')}}
   if (i == obj_c) foo() ; // expected-error {{comparison between pointer and integer ('int' and 'MyClass *')}}
-  if (obj_c == j) foo() ; // expected-error {{invalid operands to binary expression ('MyClass *' and 'int *')}}
-  if (j == obj_c) foo() ; // expected-error {{invalid operands to binary expression ('int *' and 'MyClass *')}}
+  if (obj_c == j) foo() ; // expected-error {{comparison of distinct pointer types ('MyClass *' and 'int *')}}
+  if (j == obj_c) foo() ; // expected-error {{comparison of distinct pointer types ('int *' and 'MyClass *')}}
 
   if (obj_p == i) foo() ; // expected-error {{comparison between pointer and integer ('id' and 'int')}}
   if (i == obj_p) foo() ; // expected-error {{comparison between pointer and integer ('int' and 'id')}}
-  if (obj_p == j) foo() ; // expected-error {{invalid operands to binary expression ('id' and 'int *')}}
-  if (j == obj_p) foo() ; // expected-error {{invalid operands to binary expression ('int *' and 'id')}}
+  if (obj_p == j) foo() ; // expected-error {{comparison of distinct pointer types ('id' and 'int *')}}
+  if (j == obj_p) foo() ; // expected-error {{comparison of distinct pointer types ('int *' and 'id')}}
 
   if (obj_C == i) foo() ; // expected-error {{comparison between pointer and integer ('Class' and 'int')}}
   if (i == obj_C) foo() ; // expected-error {{comparison between pointer and integer ('int' and 'Class')}}
-  if (obj_C == j) foo() ; // expected-error {{invalid operands to binary expression ('Class' and 'int *')}}
-  if (j == obj_C) foo() ; // expected-error {{invalid operands to binary expression ('int *' and 'Class')}}
+  if (obj_C == j) foo() ; // expected-error {{comparison of di

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Note that the test-case diffs are on top of https://reviews.llvm.org/D67982, 
which I split out to make the actual change in behavior in this commit clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67983



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

[PATCH] D67982: [ObjC] Add some additional test cases around pointer conversions.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

(See https://reviews.llvm.org/D67983 for the proposed behavior change.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67982



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

[PATCH] D67573: Fix __atomic_is_lock_free's return type.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67573



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

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D67983#1723376 , @thakis wrote:

> After this, Class can no longer be used as a key type in an Obj-C dictionary 
> literal. Is that intentional?


Such code was already an on by default incompatible-pointer-types warning in 
ObjC mode. That it worked in ObjC++ was accidental.

For example:

foo.m:

  #import 
  
  int main() {
NSDictionary* d = @{[NSArray class] : @"bar"};
  }

Compiling:

  $ clang -framework Foundation -o foo foo.m
  foo.m:4:23: warning: incompatible pointer types passing 'Class' to parameter 
of type 'id _Nonnull' [-Wincompatible-pointer-types]
NSDictionary* d = @{[NSArray class] : @"bar"};
^~~
  1 warning generated.

While the default metaclass does in fact implement the one method NSCopying 
declares, it's not possible within the language to declare that the Class -- 
itself, as an instance -- implements the instance methods from the NSCopying 
protocol.

You can fix the code just by doing `@{(id)clz : val}`, since id is freely 
castable to anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67983



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

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D67983#1723681 , @rjmccall wrote:

> We could probably do a quick check to see if the class informally conforms to 
> the protocol.  `+copyWithZone` seems to be marked unavailable in ARC; not 
> sure if that would cause problems for such a check.


What kind of check did you have in mind? We might hard-code the compiler to 
think that the "Class" type "implements" NSObject/NSCopying and thus is 
implicitly convertible to `id` and `id`. That usually 
would be OK since the default metaclass in the normal runtime in fact does so. 
However, there's no such guarantee, and that kind of hardcoding seems generally 
kinda sketchy.

Given that this code was already being diagnosed for a long time in ObjC mode, 
I'm not sure that adding such a hack is really warranted. I'll add a bit to the 
release notes, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67983



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

[PATCH] D69756: [opaque pointer types] Add element type argument to IRBuilder CreatePreserveStructAccessIndex and CreatePreserveArrayAccessIndex

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Updated release notes in d11a9018b773c0359934a7989d886b02468112e4 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69756



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

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Updated release notes in d11a9018b773c0359934a7989d886b02468112e4 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67983



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

[PATCH] D69876: Allow output constraints on "asm goto"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I think -Wuninitialized (UninitializedValues.cpp) should be taught how to 
detect the use of output variables in error blocks, at least for trivial cases.

Actually, for some reason -- it looks like the warning is failing the wrong way 
right now, and emits an uninitialized use warning even where there shouldn't be 
one.

E.g. this example should be okay:

  int foo(int x) {
int y;
asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
return y;
  err:
return -1;
  }

But now warns:

  $ clang -Wall -fsyntax-only foo.c
  foo.c:4:10: warning: variable 'y' is uninitialized when used here 
[-Wuninitialized]
return y;
   ^
  foo.c:2:8: note: initialize the variable 'y' to silence this warning
int y;
 ^
  = 0
  1 warning generated.

I'd expect a warning only if the code was modified to say "return y" in the err 
block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69876



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

[PATCH] D69876: Allow output constraints on "asm goto"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Also, since this means we are no longer simply implementing according to GCC's 
documentation, I think this means we'll need a brand new section in the Clang 
docs for its inline-asm support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69876



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

[PATCH] D67573: Fix __atomic_is_lock_free's return type.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67573



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

[PATCH] D70157: Align branches within 32-Byte boundary

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

> .push_align_branch_boundary [N,] [instruction,]*

I'd like to raise again the possibility of using a more general region 
directive to denote "It is allowable to add prefixes/nops before instructions 
in this region if the assembler wants to", as I'd started discussing in 
https://reviews.llvm.org/D71238#1786885 (but let's move the discussion here).

Whether this is OK or not on a particular piece of assembly-code is likely to 
be a generic property of the code, regardless of the purpose of the 
optimization. If we're going to have multiple assembler optimizations that can 
make use of this, it would be nice to express the "OK to pad" "not OK to pad" 
property only once, rather than once for each kind of optimization which might 
make such modifications.

In particular, I'd like to look ahead towards the potential implementation of 
two other features:

1. Allowing the assembler to prefix-pad instructions in order to avoid having 
to emit a NOP for p2align directives.
2. Allowing the assembler to do other instruction-padding performance 
optimizations to avoid other DSB cacheline limits.

To be concrete, I propose:
".autopad", ".noautopad": allow/disallow the assembler to emit padding via 
inserting a nop or prefix before any instruction, as needed.
".align_branch_boundary [N,] [instruction,]": Enable branch-boundary padding 
(per previous description).

In this scheme, I'd generally expect an ".align_branch_boundary" directive to 
be specified once at the beginning of the file, and ".autopad"/".noautopad" 
directives to be sprinkled throughout the file as required.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D70157#1788418 , @reames wrote:

> In D70157#1788025 , @jyknight wrote:
>
> > > .push_align_branch_boundary [N,] [instruction,]*
> >
> > I'd like to raise again the possibility of using a more general region 
> > directive to denote "It is allowable to add prefixes/nops before 
> > instructions in this region if the assembler wants to", as I'd started 
> > discussing in https://reviews.llvm.org/D71238#1786885 (but let's move the 
> > discussion here).
>
>
> James, I think this proposal is increasing the scope of this proposal too 
> much.  It also ignores some of the use cases identified and described in the 
> writeup (i.e. the scoped semantics).  I'm open to discussing such a feature 
> more generally, but I'd prefer to see a more narrowly focused feature 
> immediately.


I do not intend that we expand the scope of the project to include any of the 
other features.

All I want is to slightly consider surrounding features when adding the new 
assembly syntax. The situations where we want to avoid modifying a certain 
block of code are extremely likely to apply to //any// 
nop-or-prefix-introducing code modifications -- not just modifications 
resulting from branch alignment. So if we can make the directives annotating 
where such changes are allowable (and conversely, where they are not) 
generally-applicable, with a very minimal amount of work, that would be nice.

I also don't understand what you mean by "it ignores [...] scoped semantics"?


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

https://reviews.llvm.org/D70157



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

[PATCH] D57664: [opaque pointer types] Fix the CallInfo passed to EmitCall in some edge cases.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: dblaikie, rsmith.
Herald added a subscriber: Anastasia.
Herald added a project: clang.

Currently, EmitCall emits a call instruction with a function type
derived from the pointee-type of the callee. This *should* be the same
as the type created from the CallInfo parameter, but in some cases an
incorrect CallInfo was being passed.

All of these fixes were discovered by the addition of the assert in
EmitCall which verifies that the passed-in CallInfo matches the
Callee's function type.

As far as I know, these issues caused no bugs at the moment, as the
correct types were ultimately being emitted. But, some would become
problematic when pointee types are removed.

List of fixes:

- arrangeCXXConstructorCall was passing an incorrect value for the number of 
Required args, when calling an inheriting constructor where the inherited 
constructor is variadic. (The inheriting constructor doesn't actually get 
passed any of the user's args, but the code was calculating it as if it did).

- arrangeFreeFunctionLikeCall was not including the count of the 
pass_object_size arguments in the count of required args.

- OpenCL uses other address spaces for the "this" pointer. However, 
commonEmitCXXMemberOrOperatorCall was not annotating the address space on the 
"this" argument of the call.

- Destructor calls were being created with EmitCXXMemberOrOperatorCall instead 
of EmitCXXDestructorCall in a few places. This was a problem because the 
calling convention sometimes has destructors returning "this" rather than void, 
and the latter function knows about that, and sets up the types properly 
(through calling arrangeCXXStructorDeclaration), while the former does not.

- generateObjCGetterBody: the 'objc_getProperty' function returns type 'id', 
but was being called as if it returned the particular property's type. (That is 
of course the *dynamic* return type, and there's a downcast immediately after.)

- OpenMP user-defined reduction functions (#pragma omp declare reduction) can 
be called with a subclass of the declared type. In such case, the call was 
being setup as if the function had been actually declared to take the subtype, 
rather than the base type.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57664

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CodeGenTypes.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/CodeGenObjC/getter-property-mismatch.m

Index: clang/test/CodeGenObjC/getter-property-mismatch.m
===
--- clang/test/CodeGenObjC/getter-property-mismatch.m
+++ clang/test/CodeGenObjC/getter-property-mismatch.m
@@ -15,6 +15,4 @@
 
 // CHECK:  [[CALL:%.*]] = tail call i8* @objc_getProperty
 // CHECK:  [[ONE:%.*]] = bitcast i8* [[CALL:%.*]] to [[T1:%.*]]*
-// CHECK:  [[TWO:%.*]] = bitcast [[T1]]* [[ONE]] to [[T2:%.*]]*
-// CHECK:  ret [[T2]]* [[TWO]]
-
+// CHECK:  ret [[T1]]* [[ONE]]
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -10747,7 +10747,8 @@
   return D;
 return nullptr;
   }))
-return SemaRef.BuildDeclRefExpr(VD, Ty, VK_LValue, Loc);
+return SemaRef.BuildDeclRefExpr(VD, VD->getType().getNonReferenceType(),
+VK_LValue, Loc);
   if (auto *VD = filterLookupForUDR(
   Lookups, [&SemaRef, Ty, Loc](ValueDecl *D) -> ValueDecl * {
 if (!D->isInvalidDecl() &&
@@ -10765,7 +10766,8 @@
  /*DiagID=*/0) !=
 Sema::AR_inaccessible) {
   SemaRef.BuildBasePathArray(Paths, BasePath);
-  return SemaRef.BuildDeclRefExpr(VD, Ty, VK_LValue, Loc);
+  return SemaRef.BuildDeclRefExpr(
+  VD, VD->getType().getNonReferenceType(), VK_LValue, Loc);
 }
   }
 }
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -1566,9 +1566,8 @@
 Callee = CGCallee::forDirect(
 CGM.getAddrOfCXXStructor(DD, getFromDtorType(Type)), GD);
 
-  CGF.EmitCXXMemberOrOperatorCall(DD, Callee, ReturnValueSlot(),
-  This.getPointer(), VTT, VTTTy,
-  nullptr, nullptr);
+  CGF.EmitCXXDestructorCall(DD, Callee, This.getPointer(), VTT, VTTTy, nullptr,
+getFromDtorType(Type));
 }
 
 void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
@@ -1766,9 +1765,8 @@
   CGCallee Callee =
   CGCallee::forVirtual(CE, GlobalDecl(Dtor, DtorType), This, Ty);
 
-  CGF.EmitCXXMemberOrOperatorCall(Dtor, Callee, ReturnValueSlot(),
-  

[PATCH] D57668: [opaque pointer types] Pass function types for runtime function calls.

[James Y Knight via Phabricator via cfe-commits]

jyknight marked an inline comment as done.
jyknight added inline comments.



Comment at: clang/lib/CodeGen/CGObjC.cpp:1028-1030
+llvm::Constant *getPropertyFn = cast(
+CGM.getObjCRuntime().GetPropertyGetFunction().getCallee());
 if (!getPropertyFn) {

dblaikie wrote:
> This code looks like it implies that the 'if' is never hit? (since cast 
> doesn't propagate null (it asserts/fails/UB on null)) - should this be 
> cast_or_null instead? Or "if(!CGM.getObjCRuntime().getPropertyGetFunction())" 
> ?)
> 
> (there are a few similar instances later on)
Indeed, good catch! I don't know if that actually happens, but this code also 
didn't even need these casts.

Removed the casts, both simplifying the code, and fixing that potential issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57668



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

[PATCH] D57664: [opaque pointer types] Fix the CallInfo passed to EmitCall in some edge cases.

[James Y Knight via Phabricator via cfe-commits]

jyknight marked an inline comment as done.
jyknight added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:3837
+// having pointee types).
+llvm::FunctionType *IRFuncTyFromInfo = 
getTypes().GetFunctionType(CallInfo);
+assert(IRFuncTy == IRFuncTyFromInfo);

dblaikie wrote:
> This will be warned as unused in a release build.
> 
> Would this be hideous if it's just all one big assert?
> 
>   assert((CallInfo.isVariadic && CallInfo.getArgStruct) || IRFuncTy == 
> getTypes().GetFunctionType(CallInfo));
> 
> (I think that's accurate?)
Clearer IMO to just put #ifndef NDEBUG around the block, so I'll do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57664



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

[PATCH] D57664: [opaque pointer types] Fix the CallInfo passed to EmitCall in some edge cases.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC353181: [opaque pointer types] Fix the CallInfo passed to 
EmitCall in some (authored by jyknight, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57664?vs=184975&id=185313#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57664

Files:
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExprCXX.cpp
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CodeGenTypes.h
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/Sema/SemaOpenMP.cpp
  test/CodeGenObjC/getter-property-mismatch.m

Index: test/CodeGenObjC/getter-property-mismatch.m
===
--- test/CodeGenObjC/getter-property-mismatch.m
+++ test/CodeGenObjC/getter-property-mismatch.m
@@ -15,6 +15,4 @@
 
 // CHECK:  [[CALL:%.*]] = tail call i8* @objc_getProperty
 // CHECK:  [[ONE:%.*]] = bitcast i8* [[CALL:%.*]] to [[T1:%.*]]*
-// CHECK:  [[TWO:%.*]] = bitcast [[T1]]* [[ONE]] to [[T2:%.*]]*
-// CHECK:  ret [[T2]]* [[TWO]]
-
+// CHECK:  ret [[T1]]* [[ONE]]
Index: lib/CodeGen/CodeGenTypes.h
===
--- lib/CodeGen/CodeGenTypes.h
+++ lib/CodeGen/CodeGenTypes.h
@@ -182,6 +182,10 @@
   /// Convert clang calling convention to LLVM callilng convention.
   unsigned ClangCallConvToLLVMCallConv(CallingConv CC);
 
+  /// Derives the 'this' type for codegen purposes, i.e. ignoring method CVR
+  /// qualification.
+  CanQualType DeriveThisType(const CXXRecordDecl *RD, const CXXMethodDecl *MD);
+
   /// ConvertType - Convert type T into a llvm::Type.
   llvm::Type *ConvertType(QualType T);
 
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1566,9 +1566,8 @@
 Callee = CGCallee::forDirect(
 CGM.getAddrOfCXXStructor(DD, getFromDtorType(Type)), GD);
 
-  CGF.EmitCXXMemberOrOperatorCall(DD, Callee, ReturnValueSlot(),
-  This.getPointer(), VTT, VTTTy,
-  nullptr, nullptr);
+  CGF.EmitCXXDestructorCall(DD, Callee, This.getPointer(), VTT, VTTTy, nullptr,
+getFromDtorType(Type));
 }
 
 void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
@@ -1766,9 +1765,8 @@
   CGCallee Callee =
   CGCallee::forVirtual(CE, GlobalDecl(Dtor, DtorType), This, Ty);
 
-  CGF.EmitCXXMemberOrOperatorCall(Dtor, Callee, ReturnValueSlot(),
-  This.getPointer(), /*ImplicitParam=*/nullptr,
-  QualType(), CE, nullptr);
+  CGF.EmitCXXDestructorCall(Dtor, Callee, This.getPointer(), nullptr,
+QualType(), nullptr, getFromDtorType(DtorType));
   return nullptr;
 }
 
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -67,10 +67,17 @@
 }
 
 /// Derives the 'this' type for codegen purposes, i.e. ignoring method CVR
-/// qualification.
-static CanQualType GetThisType(ASTContext &Context, const CXXRecordDecl *RD,
-   const CXXMethodDecl *MD) {
-  QualType RecTy = Context.getTagDeclType(RD)->getCanonicalTypeInternal();
+/// qualification. Either or both of RD and MD may be null. A null RD indicates
+/// that there is no meaningful 'this' type, and a null MD can occur when
+/// calling a method pointer.
+CanQualType CodeGenTypes::DeriveThisType(const CXXRecordDecl *RD,
+ const CXXMethodDecl *MD) {
+  QualType RecTy;
+  if (RD)
+RecTy = Context.getTagDeclType(RD)->getCanonicalTypeInternal();
+  else
+RecTy = Context.VoidTy;
+
   if (MD)
 RecTy = Context.getAddrSpaceQualType(RecTy, MD->getMethodQualifiers().getAddressSpace());
   return Context.getPointerType(CanQualType::CreateUnsafe(RecTy));
@@ -235,7 +242,7 @@
 
 /// Arrange the argument and result information for a call to an
 /// unknown C++ non-static member function of the given abstract type.
-/// (Zero value of RD means we don't have any meaningful "this" argument type,
+/// (A null RD means we don't have any meaningful "this" argument type,
 ///  so fall back to a generic pointer type).
 /// The member function must be an ordinary function, i.e. not a
 /// constructor or destructor.
@@ -246,10 +253,7 @@
   SmallVector argTypes;
 
   // Add the 'this' pointer.
-  if (RD)
-argTypes.push_back(GetThisType(Context, RD, MD));
-  else
-argTypes.push_back(Context.VoidPtrTy);
+  argTypes.push_back(DeriveThisType(RD, MD));
 
   return ::arrangeLLVMFunctionInfo(
   *this, true, argTypes,
@@ -303,7 +307,7 @@
 
   SmallVector argTypes;
   SmallVector paramInfos;
-  argTypes.push_back(GetThisType(Context, MD->getParent(), MD));
+  argTypes.push_back(DeriveThisType(MD->getParent(), MD));
 

[PATCH] D57767: [opaque pointer types] Cleanup CGBuilder's Create*GEP.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: dblaikie.
Herald added subscribers: cfe-commits, jfb, jholewinski.
Herald added a project: clang.

The various EltSize, Offset, DataLayout, and StructLayout arguments
are all computable from the Address's element type and the DataLayout
which the CGBuilder already has access to.

After having previously asserted that the computed values are the same
as those passed in, now remove the redundant arguments from
CGBuilder's Create*GEP functions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57767

Files:
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCleanup.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGNonTrivialStruct.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCGNU.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/TargetInfo.cpp

Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -3642,8 +3642,8 @@
 
 static Address EmitX86_64VAArgFromMemory(CodeGenFunction &CGF,
  Address VAListAddr, QualType Ty) {
-  Address overflow_arg_area_p = CGF.Builder.CreateStructGEP(
-  VAListAddr, 2, CharUnits::fromQuantity(8), "overflow_arg_area_p");
+  Address overflow_arg_area_p =
+  CGF.Builder.CreateStructGEP(VAListAddr, 2, "overflow_arg_area_p");
   llvm::Value *overflow_arg_area =
 CGF.Builder.CreateLoad(overflow_arg_area_p, "overflow_arg_area");
 
@@ -3714,18 +3714,14 @@
   Address gp_offset_p = Address::invalid(), fp_offset_p = Address::invalid();
   llvm::Value *gp_offset = nullptr, *fp_offset = nullptr;
   if (neededInt) {
-gp_offset_p =
-CGF.Builder.CreateStructGEP(VAListAddr, 0, CharUnits::Zero(),
-"gp_offset_p");
+gp_offset_p = CGF.Builder.CreateStructGEP(VAListAddr, 0, "gp_offset_p");
 gp_offset = CGF.Builder.CreateLoad(gp_offset_p, "gp_offset");
 InRegs = llvm::ConstantInt::get(CGF.Int32Ty, 48 - neededInt * 8);
 InRegs = CGF.Builder.CreateICmpULE(gp_offset, InRegs, "fits_in_gp");
   }
 
   if (neededSSE) {
-fp_offset_p =
-CGF.Builder.CreateStructGEP(VAListAddr, 1, CharUnits::fromQuantity(4),
-"fp_offset_p");
+fp_offset_p = CGF.Builder.CreateStructGEP(VAListAddr, 1, "fp_offset_p");
 fp_offset = CGF.Builder.CreateLoad(fp_offset_p, "fp_offset");
 llvm::Value *FitsInFP =
   llvm::ConstantInt::get(CGF.Int32Ty, 176 - neededSSE * 16);
@@ -3754,9 +3750,7 @@
   // loads than necessary. Can we clean this up?
   llvm::Type *LTy = CGF.ConvertTypeForMem(Ty);
   llvm::Value *RegSaveArea = CGF.Builder.CreateLoad(
-  CGF.Builder.CreateStructGEP(
-  VAListAddr, 3, CharUnits::fromQuantity(8) + CGF.getPointerSize()),
-  "reg_save_area");
+  CGF.Builder.CreateStructGEP(VAListAddr, 3), "reg_save_area");
 
   Address RegAddr = Address::invalid();
   if (neededInt && neededSSE) {
@@ -3782,16 +3776,13 @@
 llvm::Value *V = CGF.Builder.CreateAlignedLoad(
 TyLo, CGF.Builder.CreateBitCast(RegLoAddr, PTyLo),
 CharUnits::fromQuantity(getDataLayout().getABITypeAlignment(TyLo)));
-CGF.Builder.CreateStore(V,
-CGF.Builder.CreateStructGEP(Tmp, 0, CharUnits::Zero()));
+CGF.Builder.CreateStore(V, CGF.Builder.CreateStructGEP(Tmp, 0));
 
 // Copy the second element.
 V = CGF.Builder.CreateAlignedLoad(
 TyHi, CGF.Builder.CreateBitCast(RegHiAddr, PTyHi),
 CharUnits::fromQuantity(getDataLayout().getABITypeAlignment(TyHi)));
-CharUnits Offset = CharUnits::fromQuantity(
-   getDataLayout().getStructLayout(ST)->getElementOffset(1));
-CGF.Builder.CreateStore(V, CGF.Builder.CreateStructGEP(Tmp, 1, Offset));
+CGF.Builder.CreateStore(V, CGF.Builder.CreateStructGEP(Tmp, 1));
 
 RegAddr = CGF.Builder.CreateElementBitCast(Tmp, LTy);
   } else if (neededInt) {
@@ -3838,12 +3829,10 @@
 Tmp = CGF.Builder.CreateElementBitCast(Tmp, ST);
 V = CGF.Builder.CreateLoad(CGF.Builder.CreateElementBitCast(
 RegAddrLo, ST->getStructElementType(0)));
-CGF.Builder.CreateStore(V,
-   CGF.Builder.CreateStructGEP(Tmp, 0, CharUnits::Zero()));
+CGF.Builder.CreateStore(V, CGF.Builder.CreateStructGEP(Tmp, 0));
 V = CGF.Builder.CreateLoad(CGF.Builder.CreateElementBitCast(
 RegAddrHi, ST->getStructElementType(1)));
-CGF.Builder.CreateStore(V,
-  CGF.Builder.CreateStructGEP(Tmp, 1, CharUnits::

[PATCH] D57794: Fix MSVC constructor call extension after b92d290e48e9 (r353181).

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: thakis, rsmith.
Herald added subscribers: cfe-commits, mstorsjo.
Herald added a project: clang.

The assert added to EmitCall there was triggering in Windows Chromium
builds, due to a mismatch of the return type.

The MSVC constructor call extension (`this->Foo::Foo()`) was emitting
the constructor call from 'EmitCXXMemberOrOperatorMemberCallExpr' via
calling 'EmitCXXMemberOrOperatorCall', instead of
'EmitCXXConstructorCall'. On targets where HasThisReturn is true, that
was failing to set the proper return type in the call info.

Switching to calling EmitCXXConstructorCall also allowed removing some
code e.g. the trivial copy/move support, which is already handled in
EmitCXXConstructorCall.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57794

Files:
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/test/CodeGenCXX/constructor-direct-call.cpp

Index: clang/test/CodeGenCXX/constructor-direct-call.cpp
===
--- clang/test/CodeGenCXX/constructor-direct-call.cpp
+++ clang/test/CodeGenCXX/constructor-direct-call.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple i686-pc-mingw32 -fms-extensions -Wmicrosoft %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple i686-pc-mingw32 -fms-extensions -Wmicrosoft %s -emit-llvm -o - | FileCheck %s --check-prefix=CHECK32 --check-prefix=CHECK
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc19.11.0 -fms-extensions -Wmicrosoft %s -emit-llvm -o - | FileCheck %s --check-prefix=CHECK64 --check-prefix=CHECK
 
 class Test1 {
 public:
@@ -9,7 +10,8 @@
   Test1 var;
   var.Test1::Test1();
 
-  // CHECK:   call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i32 4, i1 false)
+  // CHECK32:   call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i32 4, i1 false)
+  // CHECK64:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i64 4, i1 false)
   var.Test1::Test1(var);
 }
 
@@ -22,13 +24,16 @@
 
 void f2() {
   // CHECK:  %var = alloca %class.Test2, align 4
-  // CHECK-NEXT:  call x86_thiscallcc void @_ZN5Test2C1Ev(%class.Test2* %var)
+  // CHECK32-NEXT:  call x86_thiscallcc void @_ZN5Test2C1Ev(%class.Test2* %var)
+  // CHECK64-NEXT:  %call = call %class.Test2* @"??0Test2@@QEAA@XZ"(%class.Test2* %var)
   Test2 var;
 
-  // CHECK-NEXT:  call x86_thiscallcc void @_ZN5Test2C1Ev(%class.Test2* %var)
+  // CHECK32-NEXT:  call x86_thiscallcc void @_ZN5Test2C1Ev(%class.Test2* %var)
+  // CHECK64-NEXT:  %call1 = call %class.Test2* @"??0Test2@@QEAA@XZ"(%class.Test2* %var)
   var.Test2::Test2();
 
-  // CHECK:  call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i32 8, i1 false)
+  // CHECK32:  call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i32 8, i1 false)
+  // CHECK64:  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i64 8, i1 false)
   var.Test2::Test2(var);
 }
 
@@ -45,15 +50,19 @@
 };
 
 void f3() {
-  // CHECK: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var)
+  // CHECK32: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var)
+  // CHECK64: %call = call %class.Test3* @"??0Test3@@QEAA@XZ"(%class.Test3* %var)
   Test3 var;
 
-  // CHECK-NEXT: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var2)
+  // CHECK32-NEXT: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var2)
+  // CHECK64-NEXT: %call1 = call %class.Test3* @"??0Test3@@QEAA@XZ"(%class.Test3* %var2)
   Test3 var2;
 
-  // CHECK-NEXT: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var)
+  // CHECK32-NEXT: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var)
+  // CHECK64-NEXT: %call2 = call %class.Test3* @"??0Test3@@QEAA@XZ"(%class.Test3* %var)
   var.Test3::Test3();
 
-  // CHECK-NEXT: call x86_thiscallcc void @_ZN5Test3C1ERKS_(%class.Test3* %var, %class.Test3* dereferenceable({{[0-9]+}}) %var2)
+  // CHECK32-NEXT: call x86_thiscallcc void @_ZN5Test3C1ERKS_(%class.Test3* %var, %class.Test3* dereferenceable({{[0-9]+}}) %var2)
+  // CHECK64-NEXT: %call3 = call %class.Test3* @"??0Test3@@QEAA@AEBV0@@Z"(%class.Test3* %var, %class.Test3* dereferenceable({{[0-9]+}}) %var2)
   var.Test3::Test3(var2);
 }
Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -249,13 +249,25 @@
 This = EmitLValue(Base);
   }
 
+  if (const CXXConstructorDecl *Ctor = dyn_cast(MD)) {
+// This is the MSVC p->Ctor::Ctor(...) extension. We assume that's
+// constructing a new complete object of type Ctor.
+assert(!RtlArgs);
+assert(ReturnValue.isNull() && "Constructor shouldn't have return value");
+CallArgList Args;
+commonEmitCXXMemberOrOperatorCall(
+*this, Ctor, This.getPointer(), /*ImplicitParam=*/nullptr,
+/*ImplicitParamTy=*/QualType(), CE, A

[PATCH] D57794: Fix MSVC constructor call extension after b92d290e48e9 (r353181).

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353246: Fix MSVC constructor call extension after 
b92d290e48e9 (r353181). (authored by jyknight, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57794?vs=185426&id=185430#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57794

Files:
  cfe/trunk/lib/CodeGen/CGExprCXX.cpp
  cfe/trunk/test/CodeGenCXX/constructor-direct-call.cpp

Index: cfe/trunk/test/CodeGenCXX/constructor-direct-call.cpp
===
--- cfe/trunk/test/CodeGenCXX/constructor-direct-call.cpp
+++ cfe/trunk/test/CodeGenCXX/constructor-direct-call.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple i686-pc-mingw32 -fms-extensions -Wmicrosoft %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple i686-pc-mingw32 -fms-extensions -Wmicrosoft %s -emit-llvm -o - | FileCheck %s --check-prefix=CHECK32 --check-prefix=CHECK
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc19.11.0 -fms-extensions -Wmicrosoft %s -emit-llvm -o - | FileCheck %s --check-prefix=CHECK64 --check-prefix=CHECK
 
 class Test1 {
 public:
@@ -9,7 +10,8 @@
   Test1 var;
   var.Test1::Test1();
 
-  // CHECK:   call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i32 4, i1 false)
+  // CHECK32:   call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i32 4, i1 false)
+  // CHECK64:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i64 4, i1 false)
   var.Test1::Test1(var);
 }
 
@@ -22,13 +24,16 @@
 
 void f2() {
   // CHECK:  %var = alloca %class.Test2, align 4
-  // CHECK-NEXT:  call x86_thiscallcc void @_ZN5Test2C1Ev(%class.Test2* %var)
+  // CHECK32-NEXT:  call x86_thiscallcc void @_ZN5Test2C1Ev(%class.Test2* %var)
+  // CHECK64-NEXT:  %call = call %class.Test2* @"??0Test2@@QEAA@XZ"(%class.Test2* %var)
   Test2 var;
 
-  // CHECK-NEXT:  call x86_thiscallcc void @_ZN5Test2C1Ev(%class.Test2* %var)
+  // CHECK32-NEXT:  call x86_thiscallcc void @_ZN5Test2C1Ev(%class.Test2* %var)
+  // CHECK64-NEXT:  %call1 = call %class.Test2* @"??0Test2@@QEAA@XZ"(%class.Test2* %var)
   var.Test2::Test2();
 
-  // CHECK:  call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i32 8, i1 false)
+  // CHECK32:  call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i32 8, i1 false)
+  // CHECK64:  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %{{.*}}, i8* align 4 %{{.*}}, i64 8, i1 false)
   var.Test2::Test2(var);
 }
 
@@ -45,15 +50,19 @@
 };
 
 void f3() {
-  // CHECK: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var)
+  // CHECK32: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var)
+  // CHECK64: %call = call %class.Test3* @"??0Test3@@QEAA@XZ"(%class.Test3* %var)
   Test3 var;
 
-  // CHECK-NEXT: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var2)
+  // CHECK32-NEXT: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var2)
+  // CHECK64-NEXT: %call1 = call %class.Test3* @"??0Test3@@QEAA@XZ"(%class.Test3* %var2)
   Test3 var2;
 
-  // CHECK-NEXT: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var)
+  // CHECK32-NEXT: call x86_thiscallcc void @_ZN5Test3C1Ev(%class.Test3* %var)
+  // CHECK64-NEXT: %call2 = call %class.Test3* @"??0Test3@@QEAA@XZ"(%class.Test3* %var)
   var.Test3::Test3();
 
-  // CHECK-NEXT: call x86_thiscallcc void @_ZN5Test3C1ERKS_(%class.Test3* %var, %class.Test3* dereferenceable({{[0-9]+}}) %var2)
+  // CHECK32-NEXT: call x86_thiscallcc void @_ZN5Test3C1ERKS_(%class.Test3* %var, %class.Test3* dereferenceable({{[0-9]+}}) %var2)
+  // CHECK64-NEXT: %call3 = call %class.Test3* @"??0Test3@@QEAA@AEBV0@@Z"(%class.Test3* %var, %class.Test3* dereferenceable({{[0-9]+}}) %var2)
   var.Test3::Test3(var2);
 }
Index: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
===
--- cfe/trunk/lib/CodeGen/CGExprCXX.cpp
+++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp
@@ -249,13 +249,25 @@
 This = EmitLValue(Base);
   }
 
+  if (const CXXConstructorDecl *Ctor = dyn_cast(MD)) {
+// This is the MSVC p->Ctor::Ctor(...) extension. We assume that's
+// constructing a new complete object of type Ctor.
+assert(!RtlArgs);
+assert(ReturnValue.isNull() && "Constructor shouldn't have return value");
+CallArgList Args;
+commonEmitCXXMemberOrOperatorCall(
+*this, Ctor, This.getPointer(), /*ImplicitParam=*/nullptr,
+/*ImplicitParamTy=*/QualType(), CE, Args, nullptr);
+
+EmitCXXConstructorCall(Ctor, Ctor_Complete, /*ForVirtualBase=*/false,
+   /*Delegating=*/false, This.getAddress(), Args,
+   AggValueSlot::DoesNotOverlap, CE->getExprLoc(),
+   /*NewPointerIsChecked=*/false);
+return 

[PATCH] D57801: [opaque pointer types] Pass through function types for TLS initialization and global destructor calls.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: dblaikie.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57801

Files:
  clang/lib/CodeGen/CGCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp

Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -394,7 +394,8 @@
llvm::GlobalVariable *DeclPtr,
bool PerformInit) override;
   void registerGlobalDtor(CodeGenFunction &CGF, const VarDecl &D,
-  llvm::Constant *Dtor, llvm::Constant *Addr) override;
+  llvm::FunctionCallee Dtor,
+  llvm::Constant *Addr) override;
 
   //  Notes on array cookies =
   //
@@ -,7 +2223,7 @@
 }
 
 static void emitGlobalDtorWithTLRegDtor(CodeGenFunction &CGF, const VarDecl &VD,
-llvm::Constant *Dtor,
+llvm::FunctionCallee Dtor,
 llvm::Constant *Addr) {
   // Create a function which calls the destructor.
   llvm::Constant *DtorStub = CGF.createAtExitStub(VD, Dtor, Addr);
@@ -2241,7 +2242,7 @@
 }
 
 void MicrosoftCXXABI::registerGlobalDtor(CodeGenFunction &CGF, const VarDecl &D,
- llvm::Constant *Dtor,
+ llvm::FunctionCallee Dtor,
  llvm::Constant *Addr) {
   if (D.isNoDestroy(CGM.getContext()))
 return;
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -328,7 +328,8 @@
llvm::GlobalVariable *DeclPtr,
bool PerformInit) override;
   void registerGlobalDtor(CodeGenFunction &CGF, const VarDecl &D,
-  llvm::Constant *dtor, llvm::Constant *addr) override;
+  llvm::FunctionCallee dtor,
+  llvm::Constant *addr) override;
 
   llvm::Function *getOrCreateThreadLocalWrapper(const VarDecl *VD,
 llvm::Value *Val);
@@ -2284,9 +2285,8 @@
 
 /// Register a global destructor using __cxa_atexit.
 static void emitGlobalDtorWithCXAAtExit(CodeGenFunction &CGF,
-llvm::Constant *dtor,
-llvm::Constant *addr,
-bool TLS) {
+llvm::FunctionCallee dtor,
+llvm::Constant *addr, bool TLS) {
   const char *Name = "__cxa_atexit";
   if (TLS) {
 const llvm::Triple &T = CGF.getTarget().getTriple();
@@ -2322,11 +2322,10 @@
 // function.
 addr = llvm::Constant::getNullValue(CGF.Int8PtrTy);
 
-  llvm::Value *args[] = {
-llvm::ConstantExpr::getBitCast(dtor, dtorTy),
-llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
-handle
-  };
+  llvm::Value *args[] = {llvm::ConstantExpr::getBitCast(
+ cast(dtor.getCallee()), dtorTy),
+ llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
+ handle};
   CGF.EmitNounwindRuntimeCall(atexit, args);
 }
 
@@ -2375,9 +2374,8 @@
 }
 
 /// Register a global destructor as best as we know how.
-void ItaniumCXXABI::registerGlobalDtor(CodeGenFunction &CGF,
-   const VarDecl &D,
-   llvm::Constant *dtor,
+void ItaniumCXXABI::registerGlobalDtor(CodeGenFunction &CGF, const VarDecl &D,
+   llvm::FunctionCallee dtor,
llvm::Constant *addr) {
   if (D.isNoDestroy(CGM.getContext()))
 return;
@@ -2541,6 +2539,8 @@
   getMangleContext().mangleItaniumThreadLocalInit(VD, Out);
 }
 
+llvm::FunctionType *InitFnTy = llvm::FunctionType::get(CGM.VoidTy, false);
+
 // If we have a definition for the variable, emit the initialization
 // function as an alias to the global Init function (if any). Otherwise,
 // produce a declaration of the initialization function.
@@ -2559,8 +2559,7 @@
   // This function will not exist if the TU defining the thread_local
   // variable in question does not need any dynamic initialization for
   // its thread_local variables.
-  llvm::FunctionType *FnTy = llvm::FunctionType::get(CGM.VoidTy, false);
-  Init = llvm::Function::Crea

[PATCH] D57804: [opaque pointer types] Make EmitCall pass Function Types to CreateCall/Invoke.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: dblaikie.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Also, remove the getFunctionType() function from CGCallee, since it
accesses the pointee type of the value. The only use was in EmitCall,
so just inline it into the debug assertion.

This is the last of the changes for Call and Invoke in clang.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57804

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h

Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -204,12 +204,9 @@
   assert(isVirtual());
   return VirtualInfo.Addr;
 }
-
-llvm::FunctionType *getFunctionType() const {
-  if (isVirtual())
-return VirtualInfo.FTy;
-  return cast(
-  getFunctionPointer()->getType()->getPointerElementType());
+llvm::FunctionType *getVirtualFunctionType() const {
+  assert(isVirtual());
+  return VirtualInfo.FTy;
 }
 
 /// If this is a delayed callee computation of some sort, prepare
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -3826,7 +3826,7 @@
   QualType RetTy = CallInfo.getReturnType();
   const ABIArgInfo &RetAI = CallInfo.getReturnInfo();
 
-  llvm::FunctionType *IRFuncTy = Callee.getFunctionType();
+  llvm::FunctionType *IRFuncTy = getTypes().GetFunctionType(CallInfo);
 
 #ifndef NDEBUG
   if (!(CallInfo.isVariadic() && CallInfo.getArgStruct())) {
@@ -3837,8 +3837,13 @@
 //
 // In other cases, we assert that the types match up (until pointers stop
 // having pointee types).
-llvm::FunctionType *IRFuncTyFromInfo = getTypes().GetFunctionType(CallInfo);
-assert(IRFuncTy == IRFuncTyFromInfo);
+llvm::Type *TypeFromVal;
+if (Callee.isVirtual())
+  TypeFromVal = Callee.getVirtualFunctionType();
+else
+  TypeFromVal =
+  Callee.getFunctionPointer()->getType()->getPointerElementType();
+assert(IRFuncTy == TypeFromVal);
   }
 #endif
 
@@ -4207,8 +4212,8 @@
   // cases, we can't do any parameter mismatch checks.  Give up and bitcast
   // the callee.
   unsigned CalleeAS = CalleePtr->getType()->getPointerAddressSpace();
-  auto FnTy = getTypes().GetFunctionType(CallInfo)->getPointerTo(CalleeAS);
-  CalleePtr = Builder.CreateBitCast(CalleePtr, FnTy);
+  CalleePtr =
+  Builder.CreateBitCast(CalleePtr, IRFuncTy->getPointerTo(CalleeAS));
 } else {
   llvm::Type *LastParamTy =
   IRFuncTy->getParamType(IRFuncTy->getNumParams() - 1);
@@ -4240,19 +4245,20 @@
   //
   // This makes the IR nicer, but more importantly it ensures that we
   // can inline the function at -O0 if it is marked always_inline.
-  auto simplifyVariadicCallee = [](llvm::Value *Ptr) -> llvm::Value* {
-llvm::FunctionType *CalleeFT =
-  cast(Ptr->getType()->getPointerElementType());
+  auto simplifyVariadicCallee = [](llvm::FunctionType *CalleeFT,
+   llvm::Value *Ptr) -> llvm::Function * {
 if (!CalleeFT->isVarArg())
-  return Ptr;
+  return nullptr;
 
-llvm::ConstantExpr *CE = dyn_cast(Ptr);
-if (!CE || CE->getOpcode() != llvm::Instruction::BitCast)
-  return Ptr;
+// Get underlying value if it's a bitcast
+if (llvm::ConstantExpr *CE = dyn_cast(Ptr)) {
+  if (CE->getOpcode() == llvm::Instruction::BitCast)
+Ptr = CE->getOperand(0);
+}
 
-llvm::Function *OrigFn = dyn_cast(CE->getOperand(0));
+llvm::Function *OrigFn = dyn_cast(Ptr);
 if (!OrigFn)
-  return Ptr;
+  return nullptr;
 
 llvm::FunctionType *OrigFT = OrigFn->getFunctionType();
 
@@ -4261,15 +4267,19 @@
 if (OrigFT->isVarArg() ||
 OrigFT->getNumParams() != CalleeFT->getNumParams() ||
 OrigFT->getReturnType() != CalleeFT->getReturnType())
-  return Ptr;
+  return nullptr;
 
 for (unsigned i = 0, e = OrigFT->getNumParams(); i != e; ++i)
   if (OrigFT->getParamType(i) != CalleeFT->getParamType(i))
-return Ptr;
+return nullptr;
 
 return OrigFn;
   };
-  CalleePtr = simplifyVariadicCallee(CalleePtr);
+
+  if (llvm::Function *OrigFn = simplifyVariadicCallee(IRFuncTy, CalleePtr)) {
+CalleePtr = OrigFn;
+IRFuncTy = OrigFn->getFunctionType();
+  }
 
   // 3. Perform the actual call.
 
@@ -4364,10 +4374,10 @@
   // Emit the actual call/invoke instruction.
   llvm::CallBase *CI;
   if (!InvokeDest) {
-CI = Builder.CreateCall(CalleePtr, IRCallArgs, BundleList);
+CI = Builder.CreateCall(IRFuncTy, CalleePtr, IRCallArgs, BundleList);
   } else {
 llvm::BasicBlock *Cont = createBasicBlock("invoke.cont");
-CI = Builder.CreateInvoke(CalleePtr, Cont, InvokeDest, IRCallArgs,
+CI = Builder.CreateInvoke(IRFuncTy, Calle

[PATCH] D57801: [opaque pointer types] Pass through function types for TLS initialization and global destructor calls.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC353355: [opaque pointer types] Pass through function types 
for TLS (authored by jyknight, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57801?vs=185467&id=185677#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57801

Files:
  lib/CodeGen/CGCXX.cpp
  lib/CodeGen/CGCXXABI.h
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp

Index: lib/CodeGen/CGCXX.cpp
===
--- lib/CodeGen/CGCXX.cpp
+++ lib/CodeGen/CGCXX.cpp
@@ -227,10 +227,11 @@
   return Fn;
 }
 
-llvm::Constant *CodeGenModule::getAddrOfCXXStructor(
+llvm::FunctionCallee CodeGenModule::getAddrAndTypeOfCXXStructor(
 const CXXMethodDecl *MD, StructorType Type, const CGFunctionInfo *FnInfo,
 llvm::FunctionType *FnType, bool DontDefer,
 ForDefinition_t IsForDefinition) {
+
   GlobalDecl GD;
   if (auto *CD = dyn_cast(MD)) {
 GD = GlobalDecl(CD, toCXXCtorType(Type));
@@ -249,9 +250,10 @@
 FnType = getTypes().GetFunctionType(*FnInfo);
   }
 
-  return GetOrCreateLLVMFunction(
+  llvm::Constant *Ptr = GetOrCreateLLVMFunction(
   getMangledName(GD), FnType, GD, /*ForVTable=*/false, DontDefer,
   /*isThunk=*/false, /*ExtraAttrs=*/llvm::AttributeList(), IsForDefinition);
+  return {FnType, Ptr};
 }
 
 static CGCallee BuildAppleKextVirtualCall(CodeGenFunction &CGF,
Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -394,7 +394,8 @@
llvm::GlobalVariable *DeclPtr,
bool PerformInit) override;
   void registerGlobalDtor(CodeGenFunction &CGF, const VarDecl &D,
-  llvm::Constant *Dtor, llvm::Constant *Addr) override;
+  llvm::FunctionCallee Dtor,
+  llvm::Constant *Addr) override;
 
   //  Notes on array cookies =
   //
@@ -,7 +2223,7 @@
 }
 
 static void emitGlobalDtorWithTLRegDtor(CodeGenFunction &CGF, const VarDecl &VD,
-llvm::Constant *Dtor,
+llvm::FunctionCallee Dtor,
 llvm::Constant *Addr) {
   // Create a function which calls the destructor.
   llvm::Constant *DtorStub = CGF.createAtExitStub(VD, Dtor, Addr);
@@ -2241,7 +2242,7 @@
 }
 
 void MicrosoftCXXABI::registerGlobalDtor(CodeGenFunction &CGF, const VarDecl &D,
- llvm::Constant *Dtor,
+ llvm::FunctionCallee Dtor,
  llvm::Constant *Addr) {
   if (D.isNoDestroy(CGM.getContext()))
 return;
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3924,12 +3924,12 @@
   void EmitCXXGlobalVarDeclInit(const VarDecl &D, llvm::Constant *DeclPtr,
 bool PerformInit);
 
-  llvm::Constant *createAtExitStub(const VarDecl &VD, llvm::Constant *Dtor,
+  llvm::Function *createAtExitStub(const VarDecl &VD, llvm::FunctionCallee Dtor,
llvm::Constant *Addr);
 
   /// Call atexit() with a function that passes the given argument to
   /// the given function.
-  void registerGlobalDtorWithAtExit(const VarDecl &D, llvm::Constant *fn,
+  void registerGlobalDtorWithAtExit(const VarDecl &D, llvm::FunctionCallee fn,
 llvm::Constant *addr);
 
   /// Call atexit() with function dtorStub.
@@ -3962,8 +3962,8 @@
   /// variables.
   void GenerateCXXGlobalDtorsFunc(
   llvm::Function *Fn,
-  const std::vector>
-  &DtorsAndObjects);
+  const std::vector> &DtorsAndObjects);
 
   void GenerateCXXGlobalVarDeclInitFunc(llvm::Function *Fn,
 const VarDecl *D,
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -330,7 +330,7 @@
   switch (M->getStorageDuration()) {
   case SD_Static:
   case SD_Thread: {
-llvm::Constant *CleanupFn;
+llvm::FunctionCallee CleanupFn;
 llvm::Constant *CleanupArg;
 if (E->getType()->isArrayType()) {
   CleanupFn = CodeGenFunction(CGF.CGM).generateDestroyHelper(
@@ -339,8 +339,8 @@
   dyn_cast_or_null(M->getExtendingDecl()));
   CleanupArg = llvm::Constant::getNullValue(CGF.Int8PtrTy);
 } else {
-  CleanupFn = CGF.CGM.getAddrOfCXXStructor(ReferenceTemporaryDtor,
-

[PATCH] D57804: [opaque pointer types] Make EmitCall pass Function Types to CreateCall/Invoke.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC353356: [opaque pointer types] Make EmitCall pass Function 
Types to (authored by jyknight, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57804?vs=185476&id=185678#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57804

Files:
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGCall.h

Index: lib/CodeGen/CGCall.h
===
--- lib/CodeGen/CGCall.h
+++ lib/CodeGen/CGCall.h
@@ -204,12 +204,9 @@
   assert(isVirtual());
   return VirtualInfo.Addr;
 }
-
-llvm::FunctionType *getFunctionType() const {
-  if (isVirtual())
-return VirtualInfo.FTy;
-  return cast(
-  getFunctionPointer()->getType()->getPointerElementType());
+llvm::FunctionType *getVirtualFunctionType() const {
+  assert(isVirtual());
+  return VirtualInfo.FTy;
 }
 
 /// If this is a delayed callee computation of some sort, prepare
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -3826,7 +3826,7 @@
   QualType RetTy = CallInfo.getReturnType();
   const ABIArgInfo &RetAI = CallInfo.getReturnInfo();
 
-  llvm::FunctionType *IRFuncTy = Callee.getFunctionType();
+  llvm::FunctionType *IRFuncTy = getTypes().GetFunctionType(CallInfo);
 
 #ifndef NDEBUG
   if (!(CallInfo.isVariadic() && CallInfo.getArgStruct())) {
@@ -3837,8 +3837,13 @@
 //
 // In other cases, we assert that the types match up (until pointers stop
 // having pointee types).
-llvm::FunctionType *IRFuncTyFromInfo = getTypes().GetFunctionType(CallInfo);
-assert(IRFuncTy == IRFuncTyFromInfo);
+llvm::Type *TypeFromVal;
+if (Callee.isVirtual())
+  TypeFromVal = Callee.getVirtualFunctionType();
+else
+  TypeFromVal =
+  Callee.getFunctionPointer()->getType()->getPointerElementType();
+assert(IRFuncTy == TypeFromVal);
   }
 #endif
 
@@ -4207,8 +4212,8 @@
   // cases, we can't do any parameter mismatch checks.  Give up and bitcast
   // the callee.
   unsigned CalleeAS = CalleePtr->getType()->getPointerAddressSpace();
-  auto FnTy = getTypes().GetFunctionType(CallInfo)->getPointerTo(CalleeAS);
-  CalleePtr = Builder.CreateBitCast(CalleePtr, FnTy);
+  CalleePtr =
+  Builder.CreateBitCast(CalleePtr, IRFuncTy->getPointerTo(CalleeAS));
 } else {
   llvm::Type *LastParamTy =
   IRFuncTy->getParamType(IRFuncTy->getNumParams() - 1);
@@ -4240,19 +4245,20 @@
   //
   // This makes the IR nicer, but more importantly it ensures that we
   // can inline the function at -O0 if it is marked always_inline.
-  auto simplifyVariadicCallee = [](llvm::Value *Ptr) -> llvm::Value* {
-llvm::FunctionType *CalleeFT =
-  cast(Ptr->getType()->getPointerElementType());
+  auto simplifyVariadicCallee = [](llvm::FunctionType *CalleeFT,
+   llvm::Value *Ptr) -> llvm::Function * {
 if (!CalleeFT->isVarArg())
-  return Ptr;
+  return nullptr;
 
-llvm::ConstantExpr *CE = dyn_cast(Ptr);
-if (!CE || CE->getOpcode() != llvm::Instruction::BitCast)
-  return Ptr;
+// Get underlying value if it's a bitcast
+if (llvm::ConstantExpr *CE = dyn_cast(Ptr)) {
+  if (CE->getOpcode() == llvm::Instruction::BitCast)
+Ptr = CE->getOperand(0);
+}
 
-llvm::Function *OrigFn = dyn_cast(CE->getOperand(0));
+llvm::Function *OrigFn = dyn_cast(Ptr);
 if (!OrigFn)
-  return Ptr;
+  return nullptr;
 
 llvm::FunctionType *OrigFT = OrigFn->getFunctionType();
 
@@ -4261,15 +4267,19 @@
 if (OrigFT->isVarArg() ||
 OrigFT->getNumParams() != CalleeFT->getNumParams() ||
 OrigFT->getReturnType() != CalleeFT->getReturnType())
-  return Ptr;
+  return nullptr;
 
 for (unsigned i = 0, e = OrigFT->getNumParams(); i != e; ++i)
   if (OrigFT->getParamType(i) != CalleeFT->getParamType(i))
-return Ptr;
+return nullptr;
 
 return OrigFn;
   };
-  CalleePtr = simplifyVariadicCallee(CalleePtr);
+
+  if (llvm::Function *OrigFn = simplifyVariadicCallee(IRFuncTy, CalleePtr)) {
+CalleePtr = OrigFn;
+IRFuncTy = OrigFn->getFunctionType();
+  }
 
   // 3. Perform the actual call.
 
@@ -4364,10 +4374,10 @@
   // Emit the actual call/invoke instruction.
   llvm::CallBase *CI;
   if (!InvokeDest) {
-CI = Builder.CreateCall(CalleePtr, IRCallArgs, BundleList);
+CI = Builder.CreateCall(IRFuncTy, CalleePtr, IRCallArgs, BundleList);
   } else {
 llvm::BasicBlock *Cont = createBasicBlock("invoke.cont");
-CI = Builder.CreateInvoke(CalleePtr, Cont, InvokeDest, IRCallArgs,
+CI = Builder.CreateInvoke(IRFuncTy, CalleePtr, Cont, InvokeDest, IRCallArgs,
   BundleList);
 EmitBlock(Cont

[PATCH] D57767: [opaque pointer types] Cleanup CGBuilder's Create*GEP.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353629: [opaque pointer types] Cleanup CGBuilder's 
Create*GEP. (authored by jyknight, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57767?vs=185335&id=186134#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57767

Files:
  cfe/trunk/lib/CodeGen/CGAtomic.cpp
  cfe/trunk/lib/CodeGen/CGBlocks.cpp
  cfe/trunk/lib/CodeGen/CGBuilder.h
  cfe/trunk/lib/CodeGen/CGBuiltin.cpp
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/CodeGen/CGCleanup.cpp
  cfe/trunk/lib/CodeGen/CGDecl.cpp
  cfe/trunk/lib/CodeGen/CGExpr.cpp
  cfe/trunk/lib/CodeGen/CGExprAgg.cpp
  cfe/trunk/lib/CodeGen/CGExprComplex.cpp
  cfe/trunk/lib/CodeGen/CGNonTrivialStruct.cpp
  cfe/trunk/lib/CodeGen/CGObjC.cpp
  cfe/trunk/lib/CodeGen/CGObjCGNU.cpp
  cfe/trunk/lib/CodeGen/CGObjCMac.cpp
  cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
  cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
  cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
  cfe/trunk/lib/CodeGen/TargetInfo.cpp

Index: cfe/trunk/lib/CodeGen/CGObjC.cpp
===
--- cfe/trunk/lib/CodeGen/CGObjC.cpp
+++ cfe/trunk/lib/CodeGen/CGObjC.cpp
@@ -158,9 +158,8 @@
 if (ALE) {
   // Emit the element and store it to the appropriate array slot.
   const Expr *Rhs = ALE->getElement(i);
-  LValue LV = MakeAddrLValue(
-  Builder.CreateConstArrayGEP(Objects, i, getPointerSize()),
-  ElementType, AlignmentSource::Decl);
+  LValue LV = MakeAddrLValue(Builder.CreateConstArrayGEP(Objects, i),
+ ElementType, AlignmentSource::Decl);
 
   llvm::Value *value = EmitScalarExpr(Rhs);
   EmitStoreThroughLValue(RValue::get(value), LV, true);
@@ -170,17 +169,15 @@
 } else {
   // Emit the key and store it to the appropriate array slot.
   const Expr *Key = DLE->getKeyValueElement(i).Key;
-  LValue KeyLV = MakeAddrLValue(
-  Builder.CreateConstArrayGEP(Keys, i, getPointerSize()),
-  ElementType, AlignmentSource::Decl);
+  LValue KeyLV = MakeAddrLValue(Builder.CreateConstArrayGEP(Keys, i),
+ElementType, AlignmentSource::Decl);
   llvm::Value *keyValue = EmitScalarExpr(Key);
   EmitStoreThroughLValue(RValue::get(keyValue), KeyLV, /*isInit=*/true);
 
   // Emit the value and store it to the appropriate array slot.
   const Expr *Value = DLE->getKeyValueElement(i).Value;
-  LValue ValueLV = MakeAddrLValue(
-  Builder.CreateConstArrayGEP(Objects, i, getPointerSize()),
-  ElementType, AlignmentSource::Decl);
+  LValue ValueLV = MakeAddrLValue(Builder.CreateConstArrayGEP(Objects, i),
+  ElementType, AlignmentSource::Decl);
   llvm::Value *valueValue = EmitScalarExpr(Value);
   EmitStoreThroughLValue(RValue::get(valueValue), ValueLV, /*isInit=*/true);
   if (TrackNeededObjects) {
@@ -1666,8 +1663,8 @@
   // Save the initial mutations value.  This is the value at an
   // address that was written into the state object by
   // countByEnumeratingWithState:objects:count:.
-  Address StateMutationsPtrPtr = Builder.CreateStructGEP(
-  StatePtr, 2, 2 * getPointerSize(), "mutationsptr.ptr");
+  Address StateMutationsPtrPtr =
+  Builder.CreateStructGEP(StatePtr, 2, "mutationsptr.ptr");
   llvm::Value *StateMutationsPtr
 = Builder.CreateLoad(StateMutationsPtrPtr, "mutationsptr");
 
@@ -1748,8 +1745,8 @@
   // Fetch the buffer out of the enumeration state.
   // TODO: this pointer should actually be invariant between
   // refreshes, which would help us do certain loop optimizations.
-  Address StateItemsPtr = Builder.CreateStructGEP(
-  StatePtr, 1, getPointerSize(), "stateitems.ptr");
+  Address StateItemsPtr =
+  Builder.CreateStructGEP(StatePtr, 1, "stateitems.ptr");
   llvm::Value *EnumStateItems =
 Builder.CreateLoad(StateItemsPtr, "stateitems");
 
Index: cfe/trunk/lib/CodeGen/CGExprComplex.cpp
===
--- cfe/trunk/lib/CodeGen/CGExprComplex.cpp
+++ cfe/trunk/lib/CodeGen/CGExprComplex.cpp
@@ -327,15 +327,12 @@
 
 Address CodeGenFunction::emitAddrOfRealComponent(Address addr,
  QualType complexType) {
-  CharUnits offset = CharUnits::Zero();
-  return Builder.CreateStructGEP(addr, 0, offset, addr.getName() + ".realp");
+  return Builder.CreateStructGEP(addr, 0, addr.getName() + ".realp");
 }
 
 Address CodeGenFunction::emitAddrOfImagComponent(Address addr,
  QualType complexType) {
-  QualType eltType = complexType->castAs()->getElementType();
-  CharUnits offset = getContext().getTypeSizeInC

[PATCH] D58120: [Builtins] Treat `bcmp` as a builtin.

[James Y Knight via Phabricator via cfe-commits]

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

Looks reasonable to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58120



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

[PATCH] D58091: Customize warnings for missing built-in type

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I think this warning (-Wbuiltin-requires-header) doesn't really make sense as 
its own warning.

We already have two related (on-by-default) warnings.

For declarations:

  test.c:1:6: warning: incompatible redeclaration of library function 'exit' 
[-Wincompatible-library-redeclaration]
  long exit(char *);
   ^
  test.c:1:6: note: 'exit' is a builtin with type 'void (int) 
__attribute__((noreturn))'

And for uses:

  test2.c:1:13: warning: implicitly declaring library function 'exit' with type 
'void (int) __attribute__((noreturn))' [-Wimplicit-function-declaration]
  int foo() { exit(0); }
  ^
  test2.c:1:13: note: include the header  or explicitly provide a 
declaration for 'exit'

I think for a declaration, if we cannot construct the appropriate type, we 
should be treating all declarations as an incompatible redeclaration, and 
explain why in an attached note, like:

  warning: incompatible redeclaration of library function 'exit' 
[-Wincompatible-library-redeclaration]
  note: missing declaration of type 'jmp_buf' for argument 1 of standard 
function signature.

For a usage, we could emit something like:

  warning: implicit declaration of library function 'setjmp' 
[-Wimplicit-function-declaration]
  note: missing declaration of type 'jmp_buf' for argument 1.
  note: include the header  or explicitly provide a declaration for 
'setjmp'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58091



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

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: lib/Basic/Targets/RISCV.h:94
+if (HasA)
+  MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 32;
+  }

MaxAtomicPromoteWidth is an incompatible ABI-change kind of thing.

We should set that to the maximum atomic size this target ABI will _EVER_ 
support with any hardware, since it changes the data layout for atomic types.

MaxAtomicInlineWidth can change based on hardware, and be increased in the 
future if other hardware is introduced, but MaxAtomicPromoteWidth shouldn't.

I think it should be set it to 64 for most 32-bit platforms, and 128 for most 
64-bit platforms.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57450



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

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: philip.pfaffe.
Herald added subscribers: cfe-commits, jdoerfert, jfb, dexonsmith, steven_wu, 
hiraditya, mehdi_amini.
Herald added projects: clang, LLVM.

Just as as llvm IR supports explicitly specifying numeric value ids
for instructions, and emits them by default in textual output, now do
the same for blocks.

This is a slightly incompatible change in the textual IR format.

Previously, llvm would parse numeric labels as string names. E.g.

  define void @f() {
br label %"55"
  55:
ret void
  }

defined a label *named* "55", even without needing to be quoted, while
the reference required quoting. Now, if you intend a block label which
looks like a value number to be a name, you must quote it in the
definition too (e.g. `"55":`).

Previously, llvm would print nameless blocks only as a comment, and
would omit it if there was no predecessor. This could cause confusion
for readers of the IR, just as unnamed instructions did prior to the
addition of "%5 = " syntax, back in 2008 (PR2480).

Now, it will always print a label for an unnamed block, with the
exception of the entry block. (IMO it may be better to print it for
the entry-block as well. However, that requires updating many more
tests.)

Thus, the following is supported, and is the canonical printing:

  define i32 @f(i32, i32) {
%3 = add i32 %0, %1
br label %4
  
  4:
ret i32 %3
  }

New test cases covering this behavior are added, and other tests
updated as required.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58548

Files:
  clang/test/CodeGenCXX/discard-name-values.cpp
  llgo/test/irgen/imports.go
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLParser.h
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/IR/AsmWriter.cpp
  llvm/test/Analysis/DominanceFrontier/new_pm_test.ll
  llvm/test/Analysis/RegionInfo/cond_loop.ll
  llvm/test/Analysis/RegionInfo/condition_forward_edge.ll
  llvm/test/Analysis/RegionInfo/condition_same_exit.ll
  llvm/test/Analysis/RegionInfo/condition_simple.ll
  llvm/test/Analysis/RegionInfo/infinite_loop.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_2.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_3.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_4.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_a.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_b.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_c.ll
  llvm/test/Analysis/RegionInfo/loop_with_condition.ll
  llvm/test/Analysis/RegionInfo/mix_1.ll
  llvm/test/Analysis/RegionInfo/paper.ll
  llvm/test/Assembler/block-labels.ll
  llvm/test/Assembler/invalid-block-label-num.ll
  llvm/test/CodeGen/X86/atomic-pointer.ll
  llvm/test/Instrumentation/AddressSanitizer/asan-masked-load-store.ll
  llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime-be.ll
  llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime.ll
  llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
  llvm/test/Instrumentation/MemorySanitizer/check_access_address.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_kernel_basic.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_x86_bts_asm.ll
  llvm/test/Instrumentation/MemorySanitizer/store-origin.ll
  llvm/test/Instrumentation/SanitizerCoverage/stack-depth.ll
  llvm/test/Transforms/GVNHoist/pr36787.ll
  llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll

Index: llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
===
--- llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
+++ llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
@@ -2,10 +2,10 @@
 
 define i32 @test(i32 %arg) #0 {
 ; CHECK-LABEL: @test
-; CHECK: ; :2
+; CHECK: 2:
 ; CHECK-NEXT:  %res.0 = phi i32 [ 1, %NodeBlock ], [ 2, %1 ]
 ; CHECK-NEXT:  br label %3
-; CHECK: ; :5
+; CHECK: 5:
 ; CHECK-NEXT:   %res.3 = phi i32 [ 0, %NewDefault ], [ %res.2, %4 ]
 ; CHECK-NEXT:   %6 = add nsw i32 %res.3, 1
 ; CHECK-NEXT:   ret i32 %6
@@ -17,23 +17,23 @@
 i32 4, label %4
   ]
 
-; :1
+1:
   br label %2
 
-; :2
+2:
   %res.0 = phi i32 [ 1, %0 ], [ 2, %1 ]
   br label %3
 
-; :3
+3:
   %res.1 = phi i32 [ 0, %0 ], [ %res.0, %2 ]
   %phitmp = add nsw i32 %res.1, 2
   br label %4
 
-; :4
+4:
   %res.2 = phi i32 [ 1, %0 ], [ %phitmp, %3 ]
   br label %5
 
-; :5
+5:
   %res.3 = phi i32 [ 0, %0 ], [ %res.2, %4 ]
   %6 = add nsw i32 %res.3, 1
   ret i32 %6
Index: llvm/test/Transforms/GVNHoist/pr36787.ll
===
--- llvm/test/Transforms/GVNHoist/pr36787.ll
+++ llvm/test/Transforms/GVNHoist/pr36787.ll
@@ -16,58 +16,58 @@
   invoke void @f0()
   to label %3 unwind label %1
 
-; :1:
+1:
   %2 = landingpad { i8*, i32 }
   catch i8* bitcast (i8** @g to i8*)
   catch i8* null
   br label %16
 
-; :3:
+3:
   br i1 

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 187963.
jyknight added a comment.

Add some wording to LangRef and clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58548

Files:
  clang/test/CodeGenCXX/discard-name-values.cpp
  llgo/test/irgen/imports.go
  llvm/docs/LangRef.rst
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLParser.h
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/IR/AsmWriter.cpp
  llvm/test/Analysis/DominanceFrontier/new_pm_test.ll
  llvm/test/Analysis/RegionInfo/cond_loop.ll
  llvm/test/Analysis/RegionInfo/condition_forward_edge.ll
  llvm/test/Analysis/RegionInfo/condition_same_exit.ll
  llvm/test/Analysis/RegionInfo/condition_simple.ll
  llvm/test/Analysis/RegionInfo/infinite_loop.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_2.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_3.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_4.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_a.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_b.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_c.ll
  llvm/test/Analysis/RegionInfo/loop_with_condition.ll
  llvm/test/Analysis/RegionInfo/mix_1.ll
  llvm/test/Analysis/RegionInfo/paper.ll
  llvm/test/Assembler/block-labels.ll
  llvm/test/Assembler/invalid-block-label-num.ll
  llvm/test/CodeGen/X86/atomic-pointer.ll
  llvm/test/Instrumentation/AddressSanitizer/asan-masked-load-store.ll
  llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime-be.ll
  llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime.ll
  llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
  llvm/test/Instrumentation/MemorySanitizer/check_access_address.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_kernel_basic.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_x86_bts_asm.ll
  llvm/test/Instrumentation/MemorySanitizer/store-origin.ll
  llvm/test/Instrumentation/SanitizerCoverage/stack-depth.ll
  llvm/test/Transforms/GVNHoist/pr36787.ll
  llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll

Index: llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
===
--- llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
+++ llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
@@ -2,10 +2,10 @@
 
 define i32 @test(i32 %arg) #0 {
 ; CHECK-LABEL: @test
-; CHECK: ; :2
+; CHECK: 2:
 ; CHECK-NEXT:  %res.0 = phi i32 [ 1, %NodeBlock ], [ 2, %1 ]
 ; CHECK-NEXT:  br label %3
-; CHECK: ; :5
+; CHECK: 5:
 ; CHECK-NEXT:   %res.3 = phi i32 [ 0, %NewDefault ], [ %res.2, %4 ]
 ; CHECK-NEXT:   %6 = add nsw i32 %res.3, 1
 ; CHECK-NEXT:   ret i32 %6
@@ -17,23 +17,23 @@
 i32 4, label %4
   ]
 
-; :1
+1:
   br label %2
 
-; :2
+2:
   %res.0 = phi i32 [ 1, %0 ], [ 2, %1 ]
   br label %3
 
-; :3
+3:
   %res.1 = phi i32 [ 0, %0 ], [ %res.0, %2 ]
   %phitmp = add nsw i32 %res.1, 2
   br label %4
 
-; :4
+4:
   %res.2 = phi i32 [ 1, %0 ], [ %phitmp, %3 ]
   br label %5
 
-; :5
+5:
   %res.3 = phi i32 [ 0, %0 ], [ %res.2, %4 ]
   %6 = add nsw i32 %res.3, 1
   ret i32 %6
Index: llvm/test/Transforms/GVNHoist/pr36787.ll
===
--- llvm/test/Transforms/GVNHoist/pr36787.ll
+++ llvm/test/Transforms/GVNHoist/pr36787.ll
@@ -16,58 +16,58 @@
   invoke void @f0()
   to label %3 unwind label %1
 
-; :1:
+1:
   %2 = landingpad { i8*, i32 }
   catch i8* bitcast (i8** @g to i8*)
   catch i8* null
   br label %16
 
-; :3:
+3:
   br i1 undef, label %4, label %10
 
-;CHECK:   :4
+;CHECK:   4:
 ;CHECK-NEXT:%5 = load i32*, i32** undef, align 8
 ;CHECK-NEXT:invoke void @f1()
 
-; :4:
+4:
   %5 = load i32*, i32** undef, align 8
   invoke void @f1()
   to label %6 unwind label %1
 
-;CHECK:   :6
+;CHECK:   6:
 ;CHECK-NEXT:%7 = load i32*, i32** undef, align 8
 ;CHECK-NEXT:%8 = load i32*, i32** undef, align 8
 
-; :6:
+6:
   %7 = load i32*, i32** undef, align 8
   %8 = load i32*, i32** undef, align 8
   br i1 true, label %9, label %17
 
-; :9:
+9:
   invoke void @f0()
   to label %10 unwind label %1
 
-; :10:
+10:
   invoke void @f2()
   to label %11 unwind label %1
 
-; :11:
+11:
   %12 = invoke signext i32 undef(i32* null, i32 signext undef, i1 zeroext undef)
   to label %13 unwind label %14
 
-; :13:
+13:
   unreachable
 
-; :14:
+14:
   %15 = landingpad { i8*, i32 }
   catch i8* bitcast (i8** @g to i8*)
   catch i8* null
   br label %16
 
-; :16:
+16:
   unreachable
 
-; :17:
+17:
   ret void
 
 ; uselistorder directives
Index: llvm/test/Instrumentation/SanitizerCoverage/stack-depth.ll
===
--- llvm/test/Instrumentation/SanitizerCoverage/stack-depth.ll
+++ llvm/test/Instrumentation/Sanitize

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D58548#1407164 , @dexonsmith wrote:

> I like this idea, and I don’t think the textual IR central is too important.  
> A few things:
>
> - Changes to the IR should always be discussed on llvm-dev.  Did this already 
> happen?


I sent a message ("Improving textual IR format for nameless blocks") 
concurrently with sending this review, and will wait a bit to make sure there's 
no objections.

> - Please update LangRef.

Done.

> - Did you write a script for updating the test cases?  If so, you might 
> consider attaching it to the RFC and linking to it from the commit message as 
> a courtesy to downstream maintainers.

Nope, I didn't; there were few enough instances that it didn't seem worth 
scripting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58548



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

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 187994.
jyknight marked 4 inline comments as done.
jyknight added a comment.

Minor tweaks per comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58548

Files:
  clang/test/CodeGenCXX/discard-name-values.cpp
  llgo/test/irgen/imports.go
  llvm/docs/LangRef.rst
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLParser.h
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/IR/AsmWriter.cpp
  llvm/test/Analysis/DominanceFrontier/new_pm_test.ll
  llvm/test/Analysis/RegionInfo/cond_loop.ll
  llvm/test/Analysis/RegionInfo/condition_forward_edge.ll
  llvm/test/Analysis/RegionInfo/condition_same_exit.ll
  llvm/test/Analysis/RegionInfo/condition_simple.ll
  llvm/test/Analysis/RegionInfo/infinite_loop.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_2.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_3.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_4.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_a.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_b.ll
  llvm/test/Analysis/RegionInfo/infinite_loop_5_c.ll
  llvm/test/Analysis/RegionInfo/loop_with_condition.ll
  llvm/test/Analysis/RegionInfo/mix_1.ll
  llvm/test/Analysis/RegionInfo/paper.ll
  llvm/test/Assembler/block-labels.ll
  llvm/test/Assembler/invalid-block-label-num.ll
  llvm/test/CodeGen/X86/atomic-pointer.ll
  llvm/test/Instrumentation/AddressSanitizer/asan-masked-load-store.ll
  llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime-be.ll
  llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime.ll
  llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll
  llvm/test/Instrumentation/MemorySanitizer/check_access_address.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_kernel_basic.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_x86_bts_asm.ll
  llvm/test/Instrumentation/MemorySanitizer/store-origin.ll
  llvm/test/Instrumentation/SanitizerCoverage/stack-depth.ll
  llvm/test/Transforms/GVNHoist/pr36787.ll
  llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll

Index: llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
===
--- llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
+++ llvm/test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll
@@ -2,10 +2,10 @@
 
 define i32 @test(i32 %arg) #0 {
 ; CHECK-LABEL: @test
-; CHECK: ; :2
+; CHECK: 2:
 ; CHECK-NEXT:  %res.0 = phi i32 [ 1, %NodeBlock ], [ 2, %1 ]
 ; CHECK-NEXT:  br label %3
-; CHECK: ; :5
+; CHECK: 5:
 ; CHECK-NEXT:   %res.3 = phi i32 [ 0, %NewDefault ], [ %res.2, %4 ]
 ; CHECK-NEXT:   %6 = add nsw i32 %res.3, 1
 ; CHECK-NEXT:   ret i32 %6
@@ -17,23 +17,23 @@
 i32 4, label %4
   ]
 
-; :1
+1:
   br label %2
 
-; :2
+2:
   %res.0 = phi i32 [ 1, %0 ], [ 2, %1 ]
   br label %3
 
-; :3
+3:
   %res.1 = phi i32 [ 0, %0 ], [ %res.0, %2 ]
   %phitmp = add nsw i32 %res.1, 2
   br label %4
 
-; :4
+4:
   %res.2 = phi i32 [ 1, %0 ], [ %phitmp, %3 ]
   br label %5
 
-; :5
+5:
   %res.3 = phi i32 [ 0, %0 ], [ %res.2, %4 ]
   %6 = add nsw i32 %res.3, 1
   ret i32 %6
Index: llvm/test/Transforms/GVNHoist/pr36787.ll
===
--- llvm/test/Transforms/GVNHoist/pr36787.ll
+++ llvm/test/Transforms/GVNHoist/pr36787.ll
@@ -16,58 +16,58 @@
   invoke void @f0()
   to label %3 unwind label %1
 
-; :1:
+1:
   %2 = landingpad { i8*, i32 }
   catch i8* bitcast (i8** @g to i8*)
   catch i8* null
   br label %16
 
-; :3:
+3:
   br i1 undef, label %4, label %10
 
-;CHECK:   :4
+;CHECK:   4:
 ;CHECK-NEXT:%5 = load i32*, i32** undef, align 8
 ;CHECK-NEXT:invoke void @f1()
 
-; :4:
+4:
   %5 = load i32*, i32** undef, align 8
   invoke void @f1()
   to label %6 unwind label %1
 
-;CHECK:   :6
+;CHECK:   6:
 ;CHECK-NEXT:%7 = load i32*, i32** undef, align 8
 ;CHECK-NEXT:%8 = load i32*, i32** undef, align 8
 
-; :6:
+6:
   %7 = load i32*, i32** undef, align 8
   %8 = load i32*, i32** undef, align 8
   br i1 true, label %9, label %17
 
-; :9:
+9:
   invoke void @f0()
   to label %10 unwind label %1
 
-; :10:
+10:
   invoke void @f2()
   to label %11 unwind label %1
 
-; :11:
+11:
   %12 = invoke signext i32 undef(i32* null, i32 signext undef, i1 zeroext undef)
   to label %13 unwind label %14
 
-; :13:
+13:
   unreachable
 
-; :14:
+14:
   %15 = landingpad { i8*, i32 }
   catch i8* bitcast (i8** @g to i8*)
   catch i8* null
   br label %16
 
-; :16:
+16:
   unreachable
 
-; :17:
+17:
   ret void
 
 ; uselistorder directives
Index: llvm/test/Instrumentation/SanitizerCoverage/stack-depth.ll
===
--- llvm/test/Instrumentation/SanitizerCoverage/stack-depth.ll
+++ llvm/test/

[PATCH] D59509: Make static constructors + destructors minsize + cold (except for in -O0)

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Looks reasonable to me.




Comment at: clang/test/CodeGen/static-attr.cpp:4
+
+// WITHOUT-NOT: cold minsize noinline
+// WITH: define internal void @__cxx_global_var_init() [[ATTR:#[0-9]]]

lebedev.ri wrote:
> This is fragile, it may have false-negative if they appear in other order.
I think we guarantee it's emitted in the same order, but it's certainly the 
case that nobody will ever notice if this is failing to fail the test, if other 
attributes get emitted in between in the future.

You can use 3 separate WITHOUT-NOT lines, instead, to avoid both of this kind 
of issue.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59509



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

[PATCH] D59711: PR41183: Don't emit Wstrict-prototypes warning for an implicit function declaration.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.
Herald added a subscriber: dexonsmith.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59711



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

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: lld/ELF/InputFiles.cpp:402
+  if (Config->DepLibs)
+if (fs::exists(Specifier))
+  Driver->addFile(Specifier, /*WithLOption=*/false);

bd1976llvm wrote:
> jyknight wrote:
> > bd1976llvm wrote:
> > > jyknight wrote:
> > > > This bit seems unfortunate. "-lfoo" won't search for a file named 
> > > > "foo", and having this code do so does not seem like a good idea. I'd 
> > > > want this feature to work just like -l.
> > > > 
> > > > Actually, I think it'd be better to preserve the "-l" prefix in the 
> > > > deplibs section. (I'd note that even if libs in the search path are 
> > > > spelled "-lfoo", that doesn't mean we need accept any other options)
> > > > 
> > > > If we want to support loading a filename which not on the search path 
> > > > (but...do we, even? That seems like a kinda scary feature to me...), 
> > > > then it'd be obvious how to spell that: "libfoo.a" and "/baz/bar/foo.a" 
> > > > would open files directly, vs "-lfoo" and "-l:foo.a" would search for 
> > > > them on the search path.
> > > What you  have described is the behavior of the INPUT linker script 
> > > command:  
> > > https://sourceware.org/binutils/docs/ld/File-Commands.html#File-Commands.
> > > 
> > > We have carefully designed this "dependent libraries" feature to avoid 
> > > mapping "comment lib" pragmas to --library command line options. My 
> > > reasons:
> > > 
> > > - I don't want the compiler doing string processing to try to satisfy the 
> > > command line parsing behaviour of the target linker.
> > > 
> > > - I don't want to couple the source code to a GNU-style linker. After all 
> > > there are other ELF linkers. Your proposal isn't merely an ELF linking 
> > > proposal, it's a proposal for ELF toolchains with GNU-like linkers (e.g. 
> > > the arm linker doesn't support the colon prefix 
> > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0474c/Cjahbdei.html).
> > > 
> > > - The syntax is #pragma comment(lib, ...) not #pragma 
> > > linker-option(library, ...) i.e. the only thing this (frankly rather 
> > > bizarre) syntax definitely implies is that the argument is related to 
> > > libraries (and comments ¯\_(ツ)_/¯); it is a bit of a stretch to interpret 
> > > "comment lib" pragmas as mapping directly to "specifying an additional 
> > > --library command line option".
> > > 
> > > - I want to avoid GNUism's and use a "general" mechanism. MSVC source 
> > > code compatibility is a usecase.
> > > 
> > > - It is important to support loading a filename which not on the search 
> > > path. For example I have seen developers use the following: #pragma 
> > > comment(lib, __FILE__"\\..\\foo.lib")
> > > 
> > > I would like the following to work on for ELF:
> > > 
> > > #pragma comment(lib, "foo") => add libfoo.a to the link.
> > > #pragma comment(lib, "foo.lib") => add foo.lib to the link
> > > #pragma comment(lib, "c:\\foo.lib") => add c:\foo.lib to the link
> > > 
> > > The way the code is now these will work on both LLD and MSVC (and I think 
> > > it will work for Apple's linker as well although I haven't checked). In 
> > > addition, we have been careful to come up with a design that can be 
> > > implemented by the GNU linkers... with the cost that some complicated 
> > > links will not be possible to do via autolinking; in these (IMO rare) 
> > > cases developers will need to use the command line directly instead.
> > > 
> > > What I am trying to accomplish is to try to keep #pragma comment(lib, 
> > > "foo") compatible across linkers. Otherwise we will be in a situation 
> > > where you will have to have the equivalent of...
> > > 
> > > #ifdef COFF_LIBRARIES:
> > > #pragma comment(lib, "/DEFAULTLIB:foo.lib")
> > > #'elif ELF_LIBRARIES_GNU_STYLE_LINKER:
> > > #pragma comment(lib, "-lfoo")
> > > #'elif DARWIN_FRAMEWORKS:
> > > #pragma comment(lib, "-framework foo")
> > > #esle
> > > #error Please specify linking model
> > > #endif
> > > 
> > > ... in your source code (or the moral equivalent somewhere else). We can 
> > > avoid this.
> > > 
> > > Also note that I am not proposing to remove the .linker-options 
> > > extension. We can add support for .linker-options to LLD in the future if 
> > > we find a usecase where developers want pragmas that map directly to the 
> > > linkers command line options for ELF. If this is a usecase then, in the 
> > > future, we can implement support for #pragma comment(linker, ) 
> > > pragmas that lower to .linker-options; #pragma comment(lib, ...) pragmas 
> > > can continue to lower to .deplibs.
> > It just seems to me a really bad idea to have a situation where specifying 
> > `#pragma comment(lib, "foo")` can either open a file named "foo" in the 
> > current directory, or search for libfoo.{a,so} in the library search path.
> > 
> > That is pretty much guaranteed to cause problems due to unintentional 
> > filename collision in the current-directory.
> > 
> Linkers al

[PATCH] D59711: PR41183: Don't emit Wstrict-prototypes warning for an implicit function declaration.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59711



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

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

There shouldn't be an empty string ("") in the asm output -- that should be a 
reference to the "l_yes" label, not the empty string. That seems very weird...

Even odder: running clang -S on the above without -fno-integrated-as emits a 
".quad .Ltmp00" (note the extra "0"!), while with -fno-integrated-as, it 
properly refers to ".Ltmp0".


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

https://reviews.llvm.org/D56571



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

[PATCH] D62268: Add back --sysroot support for darwin header search.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: ldionne, jfb.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.

Before e97b5f5cf37e 
 
([clang][Darwin] Refactor header search path logic
into the driver), both --sysroot and -isysroot worked to specify where
to look for system and C++ headers on Darwin. However, that change
caused clang to start ignoring --sysroot.

This fixes the regression, and adds tests.

(I also note that on all other platforms, clang seems to almost
completely ignore -isysroot, but that's another issue...)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62268

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/test/Driver/darwin-header-search-libcxx.cpp
  clang/test/Driver/darwin-header-search-system.cpp

Index: clang/test/Driver/darwin-header-search-system.cpp
===
--- clang/test/Driver/darwin-header-search-system.cpp
+++ clang/test/Driver/darwin-header-search-system.cpp
@@ -3,7 +3,8 @@
 // General tests that the system header search paths detected by the driver
 // and passed to CC1 are correct on Darwin platforms.
 
-// Check system headers (everything below  and )
+// Check system headers (everything below  and ).  Ensure
+// that both sysroot and isysroot are checked, and that isysroot has precedence.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
 // RUN: -target x86_64-apple-darwin \
@@ -13,6 +14,26 @@
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_and_usr_local \
 // RUN:   -DRESOURCE=%S/Inputs/resource_dir \
 // RUN:   --check-prefix=CHECK-SYSTEM %s
+//
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
+// RUN: -target x86_64-apple-darwin \
+// RUN: -ccc-install-dir %S/Inputs/basic_darwin_toolchain_no_libcxx/usr/bin \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: --sysroot %S/Inputs/basic_darwin_sdk_usr_and_usr_local \
+// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_and_usr_local \
+// RUN:   -DRESOURCE=%S/Inputs/resource_dir \
+// RUN:   --check-prefix=CHECK-SYSTEM %s
+//
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
+// RUN: -target x86_64-apple-darwin \
+// RUN: -ccc-install-dir %S/Inputs/basic_darwin_toolchain_no_libcxx/usr/bin \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: -isysroot %S/Inputs/basic_darwin_sdk_usr_and_usr_local \
+// RUN: --sysroot / \
+// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_and_usr_local \
+// RUN:   -DRESOURCE=%S/Inputs/resource_dir \
+// RUN:   --check-prefix=CHECK-SYSTEM %s
+//
 // CHECK-SYSTEM: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
 // CHECK-SYSTEM: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-SYSTEM: "-internal-isystem" "[[RESOURCE]]/include"
Index: clang/test/Driver/darwin-header-search-libcxx.cpp
===
--- clang/test/Driver/darwin-header-search-libcxx.cpp
+++ clang/test/Driver/darwin-header-search-libcxx.cpp
@@ -36,6 +36,7 @@
 
 // Check with both headers in the sysroot and headers alongside the installation
 // (the headers in  should be added after the toolchain headers).
+// Ensure that both -isysroot and --sysroot work, and that isysroot has precedence.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
 // RUN: -target x86_64-apple-darwin \
@@ -46,6 +47,28 @@
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
 // RUN:   -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
 // RUN:   --check-prefix=CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1 %s
+//
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
+// RUN: -target x86_64-apple-darwin \
+// RUN: -stdlib=libc++ \
+// RUN: -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: --sysroot %S/Inputs/basic_darwin_sdk_usr \
+// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
+// RUN:   -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
+// RUN:   --check-prefix=CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1 %s
+//
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
+// RUN: -target x86_64-apple-darwin \
+// RUN: -stdlib=libc++ \
+// RUN: -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: -isysroot %S/Inputs/basic_darwin_sdk_usr \
+// RUN: --sysroot %S/Inputs/basic_darwin_sdk_no_libcxx \
+// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
+// RUN:   -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
+// RUN:   --check-prefix=CHECK-LIBCX

[PATCH] D62268: Add back --sysroot support for darwin header search.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D62268#1512672 , @ldionne wrote:

> This LGTM.
>
> When I did the refactor, all the code was only using `-isysroot` (and never 
> accessing `--sysroot`), so I thought only `-isysroot` was relevant on Darwin. 
> Seems like I was wrong.


The isysroot argument to the cc1 compiler invocation got populated from both 
isysroot and --sysroot in the driver, so that's why the behavior changed here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62268



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

[PATCH] D62268: Add back --sysroot support for darwin header search.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC361429: Add back --sysroot support for darwin header search. 
(authored by jyknight, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D62268?vs=200813&id=200815#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D62268

Files:
  lib/Driver/ToolChains/Darwin.cpp
  lib/Driver/ToolChains/Darwin.h
  test/Driver/darwin-header-search-libcxx.cpp
  test/Driver/darwin-header-search-system.cpp

Index: lib/Driver/ToolChains/Darwin.cpp
===
--- lib/Driver/ToolChains/Darwin.cpp
+++ lib/Driver/ToolChains/Darwin.cpp
@@ -1805,13 +1805,21 @@
   }
 }
 
+// Returns the effective header sysroot path to use. This comes either from
+// -isysroot or --sysroot.
+llvm::StringRef DarwinClang::GetHeaderSysroot(const llvm::opt::ArgList &DriverArgs) const {
+  if(DriverArgs.hasArg(options::OPT_isysroot))
+return DriverArgs.getLastArgValue(options::OPT_isysroot);
+  if (!getDriver().SysRoot.empty())
+return getDriver().SysRoot;
+  return "/";
+}
+
 void DarwinClang::AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
 llvm::opt::ArgStringList &CC1Args) const {
   const Driver &D = getDriver();
 
-  llvm::StringRef Sysroot = "/";
-  if (const Arg *A = DriverArgs.getLastArg(options::OPT_isysroot))
-Sysroot = A->getValue();
+  llvm::StringRef Sysroot = GetHeaderSysroot(DriverArgs);
 
   bool NoStdInc = DriverArgs.hasArg(options::OPT_nostdinc);
   bool NoStdlibInc = DriverArgs.hasArg(options::OPT_nostdlibinc);
@@ -1897,12 +1905,7 @@
   DriverArgs.hasArg(options::OPT_nostdincxx))
 return;
 
-  llvm::SmallString<128> Sysroot;
-  if (const Arg *A = DriverArgs.getLastArg(options::OPT_isysroot)) {
-Sysroot = A->getValue();
-  } else {
-Sysroot = "/";
-  }
+  llvm::StringRef Sysroot = GetHeaderSysroot(DriverArgs);
 
   switch (GetCXXStdlibType(DriverArgs)) {
   case ToolChain::CST_Libcxx: {
Index: lib/Driver/ToolChains/Darwin.h
===
--- lib/Driver/ToolChains/Darwin.h
+++ lib/Driver/ToolChains/Darwin.h
@@ -539,6 +539,8 @@
llvm::StringRef Version,
llvm::StringRef ArchDir,
llvm::StringRef BitDir) const;
+
+  llvm::StringRef GetHeaderSysroot(const llvm::opt::ArgList &DriverArgs) const;
 };
 
 } // end namespace toolchains
Index: test/Driver/darwin-header-search-libcxx.cpp
===
--- test/Driver/darwin-header-search-libcxx.cpp
+++ test/Driver/darwin-header-search-libcxx.cpp
@@ -36,6 +36,7 @@
 
 // Check with both headers in the sysroot and headers alongside the installation
 // (the headers in  should be added after the toolchain headers).
+// Ensure that both -isysroot and --sysroot work, and that isysroot has precedence.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
 // RUN: -target x86_64-apple-darwin \
@@ -46,6 +47,28 @@
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
 // RUN:   -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
 // RUN:   --check-prefix=CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1 %s
+//
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
+// RUN: -target x86_64-apple-darwin \
+// RUN: -stdlib=libc++ \
+// RUN: -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: --sysroot %S/Inputs/basic_darwin_sdk_usr \
+// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
+// RUN:   -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
+// RUN:   --check-prefix=CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1 %s
+//
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
+// RUN: -target x86_64-apple-darwin \
+// RUN: -stdlib=libc++ \
+// RUN: -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN: -isysroot %S/Inputs/basic_darwin_sdk_usr \
+// RUN: --sysroot %S/Inputs/basic_darwin_sdk_no_libcxx \
+// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
+// RUN:   -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
+// RUN:   --check-prefix=CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1 %s
+//
 // CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
 // CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
Index: test/Driver/darwin-header-search-system.cpp
===
--- test/Driver/darwin-head

[PATCH] D62035: [AST] const-ify ObjC inherited class search

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I don't really have much to say about this, and the patch is probably fine, but 
I do note that most of the other accessors on this class also return mutable 
objects.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62035



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

[PATCH] D60748: Fix i386 struct and union parameter alignment

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I don't think this was correct (where by "correct", there, I mean "what GCC 
does", as this patch is intended to match GCC behavior).

I think this change may well break more cases than it fixes, so IMO, this 
should be reverted, until it's implemented properly.

Consider one example:

  #include 
  
  typedef __attribute__((aligned(16))) int alignedint;
  
  struct __attribute__((aligned(64))) X {
  int x;
  //alignedint y;
  //__m128 y;
  };
  void g(int x, struct X);
  
  _Static_assert(_Alignof(struct X) == 64);
  
  struct X gx;
  
  void f() {
  g(1, gx);
  }

Note that when compiling this as is GCC does _not_ align X when calling g(). 
But, as of this change, now clang does. If you uncomment either the __m128 or 
alignedint lines, and now GCC aligns to 64 bytes too.

This is because GCC's algorithm is a whole lot more complex than what you've 
implemented. See its function ix86_function_arg_boundary.

The way I interpret GCC, it's doing effectively the following:
StackAlignmentForType(T):

1. If T's alignment is < 16 bytes, return 4.
2. If T is a struct/union/array type, then:
  - recursively call StackAlignmentForType() on each member's type (note -- 
this ignores any attribute((aligned(N))) directly on the //fields// of a 
struct, but not those that appear on typedefs, or the underlying types).
  - If all of those calls return alignments < 16, then return 4.
3. Otherwise, return the alignment of T.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60748



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

[PATCH] D59711: PR41183: Don't emit Wstrict-prototypes warning for an implicit function declaration.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC360084: PR41183: Don't emit strict-prototypes warning 
for an implicit function (authored by jyknight, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59711?vs=191924&id=198341#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59711

Files:
  lib/Sema/SemaType.cpp
  test/Sema/warn-strict-prototypes.c


Index: test/Sema/warn-strict-prototypes.c
===
--- test/Sema/warn-strict-prototypes.c
+++ test/Sema/warn-strict-prototypes.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes 
-verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes 
-Wno-implicit-function-declaration -verify %s
 // RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes 
-fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 // function declaration with unspecified params
@@ -71,3 +71,9 @@
 // rdar://problem/33251668
 void foo13(...) __attribute__((overloadable));
 void foo13(...) __attribute__((overloadable)) {}
+
+// We should not generate a strict-prototype warning for an implicit
+// declaration.  Leave that up to the implicit-function-declaration warning.
+void foo14(void) {
+  foo14_call(); // no-warning
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -5010,7 +5010,10 @@
 break;
   case DeclaratorChunk::Function: {
 const DeclaratorChunk::FunctionTypeInfo &FTI = DeclType.Fun;
-if (FTI.NumParams == 0 && !FTI.isVariadic)
+// We supress the warning when there's no LParen location, as this
+// indicates the declaration was an implicit declaration, which gets
+// warned about separately via -Wimplicit-function-declaration.
+if (FTI.NumParams == 0 && !FTI.isVariadic && 
FTI.getLParenLoc().isValid())
   S.Diag(DeclType.Loc, diag::warn_strict_prototypes)
   << IsBlock
   << FixItHint::CreateInsertion(FTI.getRParenLoc(), "void");


Index: test/Sema/warn-strict-prototypes.c
===
--- test/Sema/warn-strict-prototypes.c
+++ test/Sema/warn-strict-prototypes.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes -verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes -Wno-implicit-function-declaration -verify %s
 // RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 // function declaration with unspecified params
@@ -71,3 +71,9 @@
 // rdar://problem/33251668
 void foo13(...) __attribute__((overloadable));
 void foo13(...) __attribute__((overloadable)) {}
+
+// We should not generate a strict-prototype warning for an implicit
+// declaration.  Leave that up to the implicit-function-declaration warning.
+void foo14(void) {
+  foo14_call(); // no-warning
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -5010,7 +5010,10 @@
 break;
   case DeclaratorChunk::Function: {
 const DeclaratorChunk::FunctionTypeInfo &FTI = DeclType.Fun;
-if (FTI.NumParams == 0 && !FTI.isVariadic)
+// We supress the warning when there's no LParen location, as this
+// indicates the declaration was an implicit declaration, which gets
+// warned about separately via -Wimplicit-function-declaration.
+if (FTI.NumParams == 0 && !FTI.isVariadic && FTI.getLParenLoc().isValid())
   S.Diag(DeclType.Loc, diag::warn_strict_prototypes)
   << IsBlock
   << FixItHint::CreateInsertion(FTI.getRParenLoc(), "void");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

The intricate initialization-order workarounds apparently don't work in all 
build modes, so I've updated this code to have constexpr functions and 
initializations in r355278.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57914



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

[PATCH] D58154: Add support for -fpermissive.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Hm -- in GCC, -fpermissive has nothing at all to do with 
-pedantic/-pedantic-errors, but I suppose it should be fine to do this way.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58154



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

[PATCH] D58154: Add support for -fpermissive.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

The errors disabled by this feature are default-error warnings -- you can 
already get the same effect by using -Wno-. Why is it bad to 
additionally allow -fpermissive to disable them? (If we have any diagnostics 
which are currently default-on-warnings which should not _ever_ be 
disable-able, then maybe we should just fix those?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D58154



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

[PATCH] D58154: Add support for -fpermissive.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ah -- I now understand your concern, and managing user expectations 
appropriately does seem like a potential concern.

I did not look too closely before into what exactly GCC categorized as 
"permerror" (errors that can be disabled with -fpermissive) vs what Clang 
categorized as "ExtWarn,DefaultError" diagnostics. Looking into it a little 
more now, I think there may not actually be substantial overlap between the two 
right now.

Many errors we have warning flags for are unconditional in GCC, and many that 
gcc allows disabling with -fpermissive are unconditional errors in clang.

You gave examples of the latter already, and e.g. here's a bunch of bogus code 
which you can currently build with -Wno-everything in clang, but most of which 
cannot be disabled in gcc (and I believe most if not all of these flags are 
ExtWarn, so would be disabled by this proposed -fpermissive):
https://godbolt.org/z/81NkJQ

Given that, yeah, it may not actually make sense to call this behavior 
-fpermissive.

I also can't really tell what the intent is behind classifying some diagnostics 
in Clang as ExtWarn,DefaultError and others Warning,DefaultError -- or if there 
even is any useful purpose there at all. I note with special-puzzlement the 
existence of both `ext_return_missing_expr` and `warn_return_missing_expr`.

I guess my feeling now is that perhaps we should just eliminate that 
categorization as meaningless, and just make -Wno-error=everything work 
properly (for anyone who wants to not abort the compile on broken code, but for 
whatever reason also loves to see build-spam so doesn't want -Wno-everything).


Repository:
  rC Clang

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

https://reviews.llvm.org/D58154



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

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Sorry, forgot to re-ping this in a timely manner. :)

The discussion on mailing list concluded positively, so waiting for an LG here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58548



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

[PATCH] D59346: [X86] Add gcc rotate intrinsics to ia32intrin.h

[James Y Knight via Phabricator via cfe-commits]

jyknight accepted this revision.
jyknight added a comment.

Looks good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59346



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

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356789: IR: Support parsing numeric block ids, and emit them 
in textual output. (authored by jyknight, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58548?vs=187994&id=191913#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D58548

Files:
  test/CodeGenCXX/discard-name-values.cpp


Index: test/CodeGenCXX/discard-name-values.cpp
===
--- test/CodeGenCXX/discard-name-values.cpp
+++ test/CodeGenCXX/discard-name-values.cpp
@@ -10,7 +10,7 @@
   // CHECK: br i1 %pred, label %if.then, label %if.end
 
   if (pred) {
-// DISCARDVALUE: ; :2:
+// DISCARDVALUE: 2:
 // DISCARDVALUE-NEXT: tail call void @branch()
 // DISCARDVALUE-NEXT: br label %3
 
@@ -20,7 +20,7 @@
 branch();
   }
 
-  // DISCARDVALUE: ; :3:
+  // DISCARDVALUE: 3:
   // DISCARDVALUE-NEXT: ret i1 %0
 
   // CHECK: if.end:


Index: test/CodeGenCXX/discard-name-values.cpp
===
--- test/CodeGenCXX/discard-name-values.cpp
+++ test/CodeGenCXX/discard-name-values.cpp
@@ -10,7 +10,7 @@
   // CHECK: br i1 %pred, label %if.then, label %if.end
 
   if (pred) {
-// DISCARDVALUE: ; :2:
+// DISCARDVALUE: 2:
 // DISCARDVALUE-NEXT: tail call void @branch()
 // DISCARDVALUE-NEXT: br label %3
 
@@ -20,7 +20,7 @@
 branch();
   }
 
-  // DISCARDVALUE: ; :3:
+  // DISCARDVALUE: 3:
   // DISCARDVALUE-NEXT: ret i1 %0
 
   // CHECK: if.end:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59711: PR41183: Don't emit Wstrict-prototypes warning for an implicit function declaration.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: rsmith, arphaman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This case should emit an -Wimplicit-function-declaration warning.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59711

Files:
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/warn-strict-prototypes.c


Index: clang/test/Sema/warn-strict-prototypes.c
===
--- clang/test/Sema/warn-strict-prototypes.c
+++ clang/test/Sema/warn-strict-prototypes.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes 
-verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes 
-Wno-implicit-function-declaration -verify %s
 // RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes 
-fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 // function declaration with unspecified params
@@ -71,3 +71,9 @@
 // rdar://problem/33251668
 void foo13(...) __attribute__((overloadable));
 void foo13(...) __attribute__((overloadable)) {}
+
+// We should not generate a strict-prototype warning for an implicit
+// declaration.  Leave that up to the implicit-function-declaration warning.
+void foo14(void) {
+  foo14_call(); // no-warning
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -4995,7 +4995,10 @@
 break;
   case DeclaratorChunk::Function: {
 const DeclaratorChunk::FunctionTypeInfo &FTI = DeclType.Fun;
-if (FTI.NumParams == 0 && !FTI.isVariadic)
+// We supress the warning when there's no LParen location, as this
+// indicates the declaration was an implicit declaration, which gets
+// warned about separately via -Wimplicit-function-declaration.
+if (FTI.NumParams == 0 && !FTI.isVariadic && 
FTI.getLParenLoc().isValid())
   S.Diag(DeclType.Loc, diag::warn_strict_prototypes)
   << IsBlock
   << FixItHint::CreateInsertion(FTI.getRParenLoc(), "void");


Index: clang/test/Sema/warn-strict-prototypes.c
===
--- clang/test/Sema/warn-strict-prototypes.c
+++ clang/test/Sema/warn-strict-prototypes.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes -verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes -Wno-implicit-function-declaration -verify %s
 // RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-prototypes -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 // function declaration with unspecified params
@@ -71,3 +71,9 @@
 // rdar://problem/33251668
 void foo13(...) __attribute__((overloadable));
 void foo13(...) __attribute__((overloadable)) {}
+
+// We should not generate a strict-prototype warning for an implicit
+// declaration.  Leave that up to the implicit-function-declaration warning.
+void foo14(void) {
+  foo14_call(); // no-warning
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -4995,7 +4995,10 @@
 break;
   case DeclaratorChunk::Function: {
 const DeclaratorChunk::FunctionTypeInfo &FTI = DeclType.Fun;
-if (FTI.NumParams == 0 && !FTI.isVariadic)
+// We supress the warning when there's no LParen location, as this
+// indicates the declaration was an implicit declaration, which gets
+// warned about separately via -Wimplicit-function-declaration.
+if (FTI.NumParams == 0 && !FTI.isVariadic && FTI.getLParenLoc().isValid())
   S.Diag(DeclType.Loc, diag::warn_strict_prototypes)
   << IsBlock
   << FixItHint::CreateInsertion(FTI.getRParenLoc(), "void");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D54355#1327237 , @craig.topper 
wrote:

> Here's the test case that we have https://reviews.llvm.org/P8123   gcc seems 
> to accept it at O0


Smaller test case:

  extern unsigned long long __sdt_unsp;
  
  void foo() {
__asm__ __volatile__(
"" :: "n"( (__builtin_constant_p(__sdt_unsp) || 0) ? 1 : -1));
  }


Repository:
  rC Clang

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

https://reviews.llvm.org/D54355



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

[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D54355#1328557 , @void wrote:

> The issue is that "`n`" is expecting an immediate value, but the result of 
> the ternary operator isn't calculated by the front-end, because it doesn't 
> "know" that the evaluation of `__builtin_constant_p` shouldn't be delayed (it 
> being compiled at `-O0`). I suspect that this issue will also happen with the 
> "`i`" constraint.


Indeed. We _do_ actually already know that in clang too, however -- clang 
already knows that "n" requires an immediate, and does constant expression 
evaluation for it, e.g. to expand constexpr functions. Grep for 
"requiresImmediateConstant". I guess it's just not hooked up to the __b_c_p 
correctly, though.

(Note, as a workaround, you could use `|` instead of `||`, or put the 
expression as the value of an enumeration constant).


Repository:
  rC Clang

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

https://reviews.llvm.org/D54355



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

[PATCH] D55616: Emit ASM input in a constant context

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

This seems like a good start, but not complete.

"n" and "i" both should require that their argument is a constant expression. 
For "n", it actually must be an immediate constant integer, so 
setRequiresImmediate() should be used there. For "i", you may use an lvalue 
constant as well. The way we seem to indicate that, today, is with `if 
(!Info.allowsRegister() && !Info.allowsMemory())`. However the code in that 
block does not today *require* that the evaluation as a constant succeed...and 
it should.

It should also only require that the result is an integer when 
`requiresImmediateConstant()`.

Additionally, the Sema code needs to have the same conditions as the CodeGen 
code for when a constant expression is required, but it doesn't. (before or 
after your change).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55616



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

[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

That example cannot be expected to ever evaluate the expression as "1" -- it 
doesn't in GCC, nor should it in Clang. An asm constraint of "n" or "i" (but 
not, e.g., "nr") must require a constant expression, and evaluating the 
argument as a constant expression necessarily means always evaluating it to -1.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54355



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

[PATCH] D53199: Fix the behavior of clang's -w flag.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ping.


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

https://reviews.llvm.org/D53199



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

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

>> Also, do you know if having posted this patch is agreement for licensing 
>> this code? Or do we need to get explicit agreement from the original author 
>> before committing a version of this?
>
> I've never seen that be an issue before, and I don't see enough in 
> https://llvm.org/docs/DeveloperPolicy.html#copyright-license-and-patents to 
> be clear about the answer to that.  @tstellar could perhaps answer.

Contributing the patch to phab was agreement to contribute to llvm under the 
llvm license, you do not need further permission to take over and finish and 
commit a patch from phab. Per the standard apache2 terms in the llvm license, 
"Unless You explicitly state otherwise, any Contribution intentionally 
submitted for inclusion in the Work by You to the Licensor shall be under the 
terms and conditions of this License, without any additional terms or 
conditions."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53847

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

[PATCH] D133800: [Clang 15.0.1] Downgrade implicit int and implicit function declaration to warning only

[James Y Knight via Phabricator via cfe-commits]

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

Looks correct to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133800

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

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I'm confused about this new strategy of special-casing 
`source_location::current()`. Isn't it wrong to eagerly evaluate _other_ calls 
in default args, as well? ISTM that source_location is simply _exposing_ the 
bug in where we evaluate these expressions, but that it's actually a more 
general problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129488

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

[PATCH] D132944: [clang] cleanup -fstrict-flex-arrays implementation

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:910
   if (FD->getParent()->isUnion())
-return StrictFlexArraysLevel < 2;
+return true;
   RecordDecl::field_iterator FI(

This is a functional change (which is good, but the commit message needs to be 
adjusted). In current trunk
```
union X { int x[0]; };
int foo(X*x) { return x->x[2]; }
```
built with `-fstrict-flex-arrays=2 -fsanitize=array-bounds` would incorrectly 
report ubsan error, and this change fixes that.

I think this testcase can be added to clang/test/CodeGen/bounds-checking-fam.c. 
Probably this should also be nominated for backport to the 15.x branch (for the 
first point release I expect)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132944

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

[PATCH] D132944: [clang] cleanup -fstrict-flex-arrays implementation

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

  #include 
  #include 
  
  
  consteval int fn(std::source_location sl = std::source_location::current()) {
return sl.line();
  }
  
  consteval int fn2(int line = fn()) {
return line;
  }
  
  int main() {
printf("fn=%d fn2=%d\n", fn(), fn2());
  }

I believe this should print `fn=14 fn2=14`. I don't see how we can get that by 
special-casing calls to `std::source_location::current`, and indeed, with this 
version of the patch, the program instead prints `fn=14 fn2=9`.


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

https://reviews.llvm.org/D132944

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

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

  #include 
  #include 
  
  
  consteval int fn(std::source_location sl = std::source_location::current()) {
return sl.line();
  }
  
  consteval int fn2(int line = fn()) {
return line;
  }
  
  int main() {
printf("fn=%d fn2=%d\n", fn(), fn2());
  }

I believe this should print `fn=14 fn2=14`. I don't see how we can get that by 
special-casing calls to `std::source_location::current`, and indeed, with this 
version of the patch, the program instead prints `fn=14 fn2=9`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129488

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

[PATCH] D132944: [clang] cleanup -fstrict-flex-arrays implementation

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

(please ignore the last comment, I sent it to the wrong review thread)


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

https://reviews.llvm.org/D132944

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

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D129488#3761478 , @ilya-biryukov 
wrote:

> In D129488#3761398 , @jyknight 
> wrote:
>
>> I believe this should print `fn=14 fn2=14`. I don't see how we can get that 
>> by special-casing calls to `std::source_location::current`, and indeed, with 
>> this version of the patch, the program instead prints `fn=14 fn2=9`.
>
> There is another issue in Clang that prevents it from producing correct 
> results with this patch. We should not evaluate immediate calls inside 
> default arguments of immediate function parameters as they are deemed to be 
> inside the immediate function context 
> . Clang currently evaluates these 
> calls as all parameter scopes are `PotentiallyEvaluatedIfUsed` and 
> `ImmediateFunctionContext` is a different value inside the same enumeration :)

OK...So...

If the correct rule is that consteval functions must be immediately evaluated 
even when used in a default arg of a non-consteval function (rather than 
deferring evaluation to the callsite, as would otherwise be the rule for 
default args!), then GCC's behavior for "fn3" in your example 
https://gcc.godbolt.org/z/P1x8PGsh6 makes sense. And, yea, also Clang's 
_current_ behavior for `current()` makes sense! Because `current` is not 
actually generating the source location itself -- the //actual// source 
location is coming from a builtin call in a default arg of `current()`...it's 
only a wrapper, just like `fn()` is a wrapper around `current()`.

But, "makes sense" (from implementation point of view) doesn't mean 
"useful"...which is why you're adding a special case for it. And why GCC also 
did.

This is really quite awful.

The more I look at this, the more I feel like, if all the above behaviors are 
correct, source_location::current() simply shouldn't be marked consteval at 
all. If it was constexpr, instead, it'd work great _without_ needing any 
special-casing, because the "must evaluate immediately upon seeing function 
definition even in default arg position" wouldn't apply to it. And that's the 
behavior we //want// from it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129488

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

As I commented on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836#c32,

> It doesn't make sense to have a mode in which `int array[0]` is accepted but 
> is not a flex array.
> Either that should be a compilation error (as the standard specifies), or it 
> should be a flex array. Accepting it as an extension but having it do the 
> wrong thing is not useful or helpful.
> Note that Clang has a dedicated warning flag for zero-length arrays: 
> -Wzero-length-array, so anyone who wants to prohibit them may use 
> -Werror=zero-length-array. It would be helpful for GCC could follow suit 
> there.

The -fstrict-flex-arrays=3 option should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864

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

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:2644
+
+Control which arrays are considered as flexible arrays members. 
+can be 1 (array of size 0, 1 and undefined are considered), 2 (array of size 0

Docs should also mention what the default -fno-strict-flex-arrays means -- that 
ALL sizes of trailing arrays are considered flexible array members. (I'm amazed 
that's the rule, and I never knew it. I always thought the special casing for 
FAMs was restricted to sizes 0 and 1!)

Also, since apparently different parts of the compiler have been (and will now 
continue to) use different default behaviors, may want to document that as 
well. I'm sure I don't know what the rules actually are intended to be here. 
E.g. that a macro-expansion of the size arg disables the special-behavior for 
[1] is extremely surprising!



Comment at: clang/lib/Sema/SemaChecking.cpp:15804
+
+  // FIXME: we should also allow Size = 0 here per the definition of
+  // StrictFlexArraysLevel, but that's backward incompatible with previous 
clang

Presumably the size-zero/unsized cases are already being taken care of 
elsewhere in the code? I find it hard to believe we are currently emitting 
diagnostics for size-0 FAM which we don't emit for size-1 FAM?



Comment at: clang/test/SemaCXX/array-bounds-strict-flex-arrays.cpp:9
+  } s2;
+  s2.a[2] = 0; // no-warning
+}

Except we actually _do_ know the bounds of the full-object and ought to be able 
to warn on this code anyhow...

Better to have the test function accept a pointer, so that's not a conflating 
issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864

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