[PATCH] D92797: APINotes: add initial stub of APINotesWriter

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf806be5eaae1: APINotes: add initial stub of APINotesWriter 
(authored by Saleem Abdulrasool , committed by 
compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D92797?vs=310049&id=551157#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92797

Files:
  clang/include/clang/APINotes/APINotesWriter.h
  clang/lib/APINotes/APINotesFormat.h
  clang/lib/APINotes/APINotesWriter.cpp
  clang/lib/APINotes/CMakeLists.txt

Index: clang/lib/APINotes/CMakeLists.txt
===
--- clang/lib/APINotes/CMakeLists.txt
+++ clang/lib/APINotes/CMakeLists.txt
@@ -2,6 +2,7 @@
   Support)
 add_clang_library(clangAPINotes
   APINotesTypes.cpp
+  APINotesWriter.cpp
   APINotesYAMLCompiler.cpp
   LINK_LIBS
 clangBasic)
Index: clang/lib/APINotes/APINotesWriter.cpp
===
--- /dev/null
+++ clang/lib/APINotes/APINotesWriter.cpp
@@ -0,0 +1,1165 @@
+//===-- APINotesWriter.cpp - API Notes Writer ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/APINotes/APINotesWriter.h"
+#include "APINotesFormat.h"
+#include "clang/APINotes/Types.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Bitstream/BitstreamWriter.h"
+#include "llvm/Support/DJB.h"
+#include "llvm/Support/OnDiskHashTable.h"
+#include "llvm/Support/VersionTuple.h"
+
+namespace clang {
+namespace api_notes {
+class APINotesWriter::Implementation {
+  template 
+  using VersionedSmallVector =
+  llvm::SmallVector, 1>;
+
+  std::string ModuleName;
+  const FileEntry *SourceFile;
+
+  /// Scratch space for bitstream writing.
+  llvm::SmallVector Scratch;
+
+#if defined(__APPLE__) && defined(SWIFT_DOWNSTREAM)
+  bool SwiftImportAsMember = false;
+#endif
+
+  /// Mapping from strings to identifier IDs.
+  llvm::StringMap IdentifierIDs;
+
+  /// Information about Objective-C contexts (classes or protocols).
+  ///
+  /// Indexed by the identifier ID and a bit indication whether we're looking
+  /// for a class (0) or protocol (1) and provides both the context ID and
+  /// information describing the context within that module.
+  llvm::DenseMap,
+ std::pair>>
+  ObjCContexts;
+
+  /// Information about Objective-C properties.
+  ///
+  /// Indexed by the context ID, property name, and whether this is an
+  /// instance property.
+  llvm::DenseMap<
+  std::tuple,
+  llvm::SmallVector, 1>>
+  ObjCProperties;
+
+  /// Information about Objective-C methods.
+  ///
+  /// Indexed by the context ID, selector ID, and Boolean (stored as a char)
+  /// indicating whether this is a class or instance method.
+  llvm::DenseMap,
+ llvm::SmallVector, 1>>
+  ObjCMethods;
+
+  /// Mapping from selectors to selector ID.
+  llvm::DenseMap SelectorIDs;
+
+  /// Information about global variables.
+  ///
+  /// Indexed by the identifier ID.
+  llvm::DenseMap, 1>>
+  GlobalVariables;
+
+  /// Information about global functions.
+  ///
+  /// Indexed by the identifier ID.
+  llvm::DenseMap, 1>>
+  GlobalFunctions;
+
+  /// Information about enumerators.
+  ///
+  /// Indexed by the identifier ID.
+  llvm::DenseMap<
+  unsigned, llvm::SmallVector, 1>>
+  EnumConstants;
+
+  /// Information about tags.
+  ///
+  /// Indexed by the identifier ID.
+  llvm::DenseMap, 1>>
+  Tags;
+
+  /// Information about typedefs.
+  ///
+  /// Indexed by the identifier ID.
+  llvm::DenseMap, 1>>
+  Typedefs;
+
+  void writeBlockInfoBlock(llvm::BitstreamWriter &Stream);
+  void writeControlBlock(llvm::BitstreamWriter &Stream);
+  void writeIdentifierBlock(llvm::BitstreamWriter &Stream);
+  void writeObjCContextBlock(llvm::BitstreamWriter &Stream);
+  void writeObjCPropertyBlock(llvm::BitstreamWriter &Stream);
+  void writeObjCMethodBlock(llvm::BitstreamWriter &Stream);
+  void writeObjCSelectorBlock(llvm::BitstreamWriter &Stream);
+  void writeGlobalVariableBlock(llvm::BitstreamWriter &Stream);
+  void writeGlobalFunctionBlock(llvm::BitstreamWriter &Stream);
+  void writeEnumConstantBlock(llvm::BitstreamWriter &Stream);
+  void writeTagBlock(llvm::BitstreamWriter &Stream);
+  void writeTypedefBlock(llvm::BitstreamWriter &Stream);
+
+public:
+  Implementation(llvm::StringRef ModuleName, const FileEntry *SF)
+  : ModuleName(std::string(ModuleName)), SourceFile(SF) {}
+
+  void writeToStream(llvm::raw_ostrea

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG05d613ea931b: [lit][clang] Avoid realpath on Windows due to 
MAX_PATH limitations (authored by compnerd).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/test/Lexer/case-insensitive-include-win.c
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 def map_config(source_dir, site_config):
 global config_map
-source_dir = os.path.realpath(source_dir)
+source_dir = os.path.abspath(source_dir)
 source_dir = os.path.normcase(source_dir)
 site_config = os.path.normpath(site_config)
 config_map[source_dir] = site_config
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 config.test_exec_root = None
 import os.path
 
-this_dir = os.path.realpath(os.path.dirname(__file__))
+this_dir = os.path.dirname(lit.util.abs_path_preserve_drive(__file__))
 config.llvm_tools_dir = os.path.join(this_dir, "build")
 import lit.llvm
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 config.test_exec_root = None
 import os.path
 
-config.llvm_tools_dir = os.path.realpath(os.path.dirname(__file__))
+config.llvm_tools_dir = os.path.dirname(lit.util.abs_path_preserve_drive(__file__))
 import lit.llvm
 
 lit.llvm.initialize(lit_config, config)
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@
 config.test_format = lit.formats.ShTest()
 
 import os
-config.test_exec_root = os.path.realpath(os.path.dirname(__file__))
+config.test_exec_root = os.path.dirname(lit.util.abs_path_preserve_drive(__file__))
 config.test_source_root = os.path.join(config.test_exec_root, "tests")
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
@@ -2,9 +2,7 @@
 import os
 import sys
 
-main_config = sys.argv[1]
-main_config = os.path.realpath(main_config)
-main_config = os.path.normcase(main_config)
+main_config = lit.util.abs_path_preserve_drive(sys.argv[1])
 
 config_map = {main_config: sys.argv[2]}
 builtin_parameters = {"config_map": config_map}
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -127,6 +127,23 @@
 
 return n
 
+def abs_path_preserve_drive(path):
+"""Return the absolute path without resolving drive mappings on Windows.
+
+"""
+if platform.system() == "Windows":
+# Windows has limitations on path length (MAX_PATH) that
+# can be worked around using substitute drives, which map
+# a drive letter to a longer path on another drive.
+# Since Python 3.8, os.path.realpath resolves sustitute drives,
+# so we should not use it. In Python 3.7, os.path.realpath
+# was implemented as os.path.abspath.
+return os.path.normpath(os.path.abspath(path))
+else:
+# On UNIX, the current directory always has symbolic links resolved,
+# so any program accepting relative paths cannot preserve symbolic
+# links in paths and we sho

[PATCH] D152051: libclang-cpp: Add external visibility attribute to all classes

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

In D152051#4392612 , @tstellar wrote:

> I was not sure what to do with inline functions and also functions that were 
> implemented in the headers, so I did not add the LLVM_EXTERNAL_VISIBILITY 
> macro to most of those functions.

I think that inline functions should have the attribute applied as well.  That 
should be the equivalent of `-fvisibility-inlines-hidden` and should allow this 
to be useable for windows as well.

Please do not use LLVM_EXTERNAL_VISIBILITY but rather introduce a new macro 
(this will prevent the use on Windows).

I did write a tool to help with this: https://GitHub.com/compnerd/ids


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152051

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

[PATCH] D151837: [Clang][Parser] Accept GNU attributes preceding C++ style attributes on templates

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Could you add a test case for the inverted order of attributes on a template as 
well?  I don't think that there is a test case for that.


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

https://reviews.llvm.org/D151837

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

[PATCH] D139749: Headers: use C++ inline semantics in C++ mode

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3b0677964c46: Headers: use C++ inline semantics in C++ mode 
(authored by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D139749?vs=483081&id=515351#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139749

Files:
  clang/lib/Headers/adxintrin.h


Index: clang/lib/Headers/adxintrin.h
===
--- clang/lib/Headers/adxintrin.h
+++ clang/lib/Headers/adxintrin.h
@@ -17,56 +17,69 @@
 /* Define the default attributes for the functions in this file. */
 #define __DEFAULT_FN_ATTRS __attribute__((__always_inline__, __nodebug__))
 
+/* Use C++ inline semantics in C++, GNU inline for C mode. */
+#if defined(__cplusplus)
+#define __INLINE __inline
+#else
+#define __INLINE static __inline
+#endif
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
 /* Intrinsics that are available only if __ADX__ defined */
-static __inline unsigned char __attribute__((__always_inline__, __nodebug__, 
__target__("adx")))
-_addcarryx_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
-   unsigned int *__p)
-{
+__INLINE unsigned char
+__attribute__((__always_inline__, __nodebug__, __target__("adx")))
+_addcarryx_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
+   unsigned int *__p) {
   return __builtin_ia32_addcarryx_u32(__cf, __x, __y, __p);
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __attribute__((__always_inline__, __nodebug__, 
__target__("adx")))
-_addcarryx_u64(unsigned char __cf, unsigned long long __x,
-   unsigned long long __y, unsigned long long  *__p)
-{
+__INLINE unsigned char
+__attribute__((__always_inline__, __nodebug__, __target__("adx")))
+_addcarryx_u64(unsigned char __cf, unsigned long long __x,
+   unsigned long long __y, unsigned long long *__p) {
   return __builtin_ia32_addcarryx_u64(__cf, __x, __y, __p);
 }
 #endif
 
 /* Intrinsics that are also available if __ADX__ undefined */
-static __inline unsigned char __DEFAULT_FN_ATTRS
-_addcarry_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
-  unsigned int *__p)
-{
+__INLINE unsigned char __DEFAULT_FN_ATTRS _addcarry_u32(unsigned char __cf,
+unsigned int __x,
+unsigned int __y,
+unsigned int *__p) {
   return __builtin_ia32_addcarryx_u32(__cf, __x, __y, __p);
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _addcarry_u64(unsigned char __cf, unsigned long long __x,
-  unsigned long long __y, unsigned long long  *__p)
-{
+  unsigned long long __y, unsigned long long *__p) {
   return __builtin_ia32_addcarryx_u64(__cf, __x, __y, __p);
 }
 #endif
 
-static __inline unsigned char __DEFAULT_FN_ATTRS
-_subborrow_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
-  unsigned int *__p)
-{
+__INLINE unsigned char __DEFAULT_FN_ATTRS _subborrow_u32(unsigned char __cf,
+ unsigned int __x,
+ unsigned int __y,
+ unsigned int *__p) {
   return __builtin_ia32_subborrow_u32(__cf, __x, __y, __p);
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _subborrow_u64(unsigned char __cf, unsigned long long __x,
-   unsigned long long __y, unsigned long long  *__p)
-{
+   unsigned long long __y, unsigned long long *__p) {
   return __builtin_ia32_subborrow_u64(__cf, __x, __y, __p);
 }
 #endif
 
+#if defined(__cplusplus)
+}
+#endif
+
 #undef __DEFAULT_FN_ATTRS
 
 #endif /* __ADXINTRIN_H */


Index: clang/lib/Headers/adxintrin.h
===
--- clang/lib/Headers/adxintrin.h
+++ clang/lib/Headers/adxintrin.h
@@ -17,56 +17,69 @@
 /* Define the default attributes for the functions in this file. */
 #define __DEFAULT_FN_ATTRS __attribute__((__always_inline__, __nodebug__))
 
+/* Use C++ inline semantics in C++, GNU inline for C mode. */
+#if defined(__cplusplus)
+#define __INLINE __inline
+#else
+#define __INLINE static __inline
+#endif
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
 /* Intrinsics that are available only if __ADX__ defined */
-static __inline unsigned char __attribute__((__always_inline__, __nodebug__, __target__("adx")))
-_addcarryx_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
-   unsigned int *__p)
-{
+__INLINE unsigned char
+

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG893ce5759fe2: docs: add some documentation on Windows SDK 
search (authored by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D146165?vs=506978&id=507346#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146165

Files:
  clang/docs/UsersManual.rst

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -4487,3 +4487,126 @@
 to produce DLLs and EXEs. To link a DLL, pass
 ``clang_rt.asan_dll_thunk-x86_64.lib``. To link an EXE, pass
 ``-wholearchive:clang_rt.asan-x86_64.lib``.
+
+Windows System Headers and Library Lookup
+^
+
+clang-cl uses a set of different approaches to locate the right system libraries
+to link against when building code.  The Windows environment uses libraries from
+three distinct sources:
+
+1. Windows SDK
+2. UCRT (Universal C Runtime)
+3. Visual C++ Tools (VCRuntime)
+
+The Windows SDK provides the import libraries and headers required to build
+programs against the Windows system packages.  Underlying the Windows SDK is the
+UCRT, the universal C runtime.
+
+This difference is best illustrated by the various headers that one would find
+in the different categories.  The WinSDK would contain headers such as
+`WinSock2.h` which is part of the Windows API surface, providing the Windows
+socketing interfaces for networking.  UCRT provides the C library headers,
+including e.g. `stdio.h`.  Finally, the Visual C++ tools provides the underlying
+Visual C++ Runtime headers such as `stdint.h` or `crtdefs.h`.
+
+There are various controls that allow the user control over where clang-cl will
+locate these headers.  The default behaviour for the Windows SDK and UCRT is as
+follows:
+
+1. Consult the command line.
+
+Anything the user specifies is always given precedence.  The following
+extensions are part of the clang-cl toolset:
+
+- `/winsysroot:`
+
+The `/winsysroot:` is used as an equivalent to `-sysroot` on Unix
+environments.  It allows the control of an alternate location to be treated
+as a system root.  When specified, it will be used as the root where the
+`Windows Kits` is located.
+
+- `/winsdkversion:`
+- `/winsdkdir:`
+
+If `/winsysroot:` is not specified, the `/winsdkdir:` argument is consulted
+as a location to identify where the Windows SDK is located.  Contrary to
+`/winsysroot:`, `/winsdkdir:` is expected to be the complete path rather
+than a root to locate `Windows Kits`.
+
+The `/winsdkversion:` flag allows the user to specify a version identifier
+for the SDK to prefer.  When this is specified, no additional validation is
+performed and this version is preferred.  If the version is not specified,
+the highest detected version number will be used.
+
+2. Consult the environment.
+
+TODO: This is not yet implemented.
+
+This will consult the environment variables:
+
+- `WindowsSdkDir`
+- `UCRTVersion`
+
+3. Fallback to the registry.
+
+If no arguments are used to indicate where the SDK is present, and the
+compiler is running on Windows, the registry is consulted to locate the
+installation.
+
+The Visual C++ Toolset has a slightly more elaborate mechanism for detection.
+
+1. Consult the command line.
+
+- `/winsysroot:`
+
+The `/winsysroot:` is used as an equivalent to `-sysroot` on Unix
+environments.  It allows the control of an alternate location to be treated
+as a system root.  When specified, it will be used as the root where the
+`VC` directory is located.
+
+- `/vctoolsdir:`
+- `/vctoolsversion:`
+
+If `/winsysroot:` is not specified, the `/vctoolsdir:` argument is consulted
+as a location to identify where the Visual C++ Tools are located.  If
+`/vctoolsversion:` is specified, that version is preferred, otherwise, the
+highest version detected is used.
+
+2. Consult the environment.
+
+- `/external:[VARIABLE]`
+
+  This specifies a user identified environment variable which is treated as
+  a path delimiter (`;`) separated list of paths to map into `-imsvc`
+  arguments which are treated as `-isystem`.
+
+- `INCLUDE` and `EXTERNAL_INCLUDE`
+
+  The path delimiter (`;`) separated list of paths will be mapped to
+  `-imsvc` arguments which are treated as `-isystem`.
+
+- `LIB` (indirectly)
+
+  The linker `link.exe` or `lld-link.exe` will honour the environment
+  variable `LIB` which is a path delimiter (`;`) set of paths to consult for
+  the import libraries to use when linking the final target.
+
+The following environment variables will be consulted and used to form paths
+to validate and load content from as appropriate:
+
+ 

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Update to include additional behaviour and reference `INCLUDE` and `LIB`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146165

Files:
  clang/docs/UsersManual.rst

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -4487,3 +4487,125 @@
 to produce DLLs and EXEs. To link a DLL, pass
 ``clang_rt.asan_dll_thunk-x86_64.lib``. To link an EXE, pass
 ``-wholearchive:clang_rt.asan-x86_64.lib``.
+
+Windows System Headers and Library Lookup
+^
+
+clang-cl uses a set of different approaches to locate the right system libraries
+to link against when building code.  The Windows environment uses libraries from
+three distinct sources:
+
+1. Windows SDK
+2. UCRT (Universal C Runtime)
+3. Visual C++ Tools (VCRuntime)
+
+The Windows SDK provides the import libraries and headers required to build
+programs against the Windows system packages.  Underlying the Windows SDK is the
+UCRT, the universal C runtime.
+
+This difference is best illustrated by the various headers that one would find
+in the different categories.  The WinSDK would contain headers such as
+`WinSock2.h` which is part of the Windows API surface, providing the Windows
+socketing interfaces for networking.  UCRT provides the C library headers,
+including e.g. `stdio.h`.  Finally, the Visual C++ tools provides the underlying
+Visual C++ Runtime headers such as `stdint.h` or `crtdefs.h`.
+
+There are various controls that allow the user control over where clang-cl will
+locate these headers.  The default behaviour for the Windows SDK and UCRT is as
+follows:
+
+1. Consult the command line.
+
+Anything the user specifies is always given precedence.  The following
+extensions are part of the clang-cl toolset:
+
+- `/winsysroot:`
+
+The `/winsysroot:` is used as an equivalent to `-sysroot` on Unix
+environments.  It allows the control of an alternate location to be treated
+as a system root.  When specified, it will be used as the root where the
+`Windows Kits` is located.
+
+- `/winsdkversion:`
+- `/winsdkdir:`
+
+If `/winsysroot:` is not specified, the `/winsdkdir:` argument is consulted
+as a location to identify where the Windows SDK is located.  Contrary to
+`/winsysroot:`, `/winsdkdir:` is expected to be the complete path rather
+than a root to locate `Windows Kits`.
+
+The `/winsdkversion:` flag allows the user to specify a version identifier
+for the SDK to prefer.  When this is specified, no additional validation is
+performed and this version is preferred.  If the version is not specified,
+the highest detected version number will be used.
+
+2. Consult the environment.
+
+TODO: This is not yet implemented.
+
+This will consult the environment variables:
+- `WindowsSdkDir`
+- `UCRTVersion`
+
+3. Fallback to the registry.
+
+If no arguments are used to indicate where the SDK is present, and the
+compiler is running on Windows, the registry is consulted to locate the
+installation.
+
+The Visual C++ Toolset has a slightly more elaborate mechanism for detection.
+
+1. Consult the command line.
+
+- `/winsysroot:`
+
+The `/winsysroot:` is used as an equivalent to `-sysroot` on Unix
+environments.  It allows the control of an alternate location to be treated
+as a system root.  When specified, it will be used as the root where the
+`VC` directory is located.
+
+- `/vctoolsdir:`
+- `/vctoolsversion:`
+
+If `/winsysroot:` is not specified, the `/vctoolsdir:` argument is consulted
+as a location to identify where the Visual C++ Tools are located.  If
+`/vctoolsversion:` is specified, that version is preferred, otherwise, the
+highest version detected is used.
+
+2. Consult the environment.
+
+- `/external:[VARIABLE]`
+
+  This specifies a user identified environment variable which is treated as
+  a path delimiter (`;`) separated list of paths to map into `-imsvc`
+  arguments which are treated as `-isystem`.
+
+- `INCLUDE` and `EXTERNAL_INCLUDE`
+
+  The path delimiter (`;`) separated list of paths will be mapped to
+  `-imsvc` arguments which are treated as `-isystem`.
+
+- `LIB` (indirectly)
+
+  The linker `link.exe` or `lld-link.exe` will honour the environment
+  variable `LIB` which is a path delimiter (`;`) set of paths to consult for
+  the import libraries to use when linking the final target.
+
+The following environment variables will be consulted and used to form paths
+to validate and load content from as appropriate:
+
+  - `VCToolsInstallDir
+  - `VCINSTALLDIR`
+  - `Path`
+
+3. Consult `ISetupConfiguration` [Windows Only]
+
+A

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd marked an inline comment as not done.
compnerd added inline comments.



Comment at: clang/docs/UsersManual.rst:4504
+programs against the Windows system packages.  Underlying the Windows SDK is 
the
+UCRT, the universal C runtime.
+

hans wrote:
> Optionally, If you wanted to be ultra pedagogical, this paragraph could 
> perhaps list an example library/header as part of explaining the SDK, UCRT, 
> and tools.
I like this idea, I'll update this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146165

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

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

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

Yes, I agree, I did miss that in this.  I don't think that the point is 
entirely correct though, because `Include` **does** impact the WinSDK lookup.  
That means that it is reflective of what the section is talking about.  `LIB` 
belongs in both I believe as that impacts the lookup for the libraries for both 
the SDK and Visual C++ tools (for VC Runtime).  Basically, I think that your 
point is more important than what you were attributing to it and this needs to 
be further refined for precision.




Comment at: clang/docs/UsersManual.rst:4537
+
+TODO: This is not yet implemented.
+

hans wrote:
> But isn't this how clang-cl finds stuff when running in a "Visual Studio 
> Command Prompt" / setenv / vcvars or whatever it's called these days?
> 
> I think this would be the most common case for people building from the 
> command line, and I'd suggest perhaps starting the list with this one (I 
> realize they're currently sorted by precedence, but sorting by most common 
> first would also be valid I think).
> But isn't this how clang-cl finds stuff when running in a "Visual Studio 
> Command Prompt" / setenv / vcvars or whatever it's called these days?

No, it isn't, because this is the Windows SDK portion.  The environment 
variable is for the Visual C++ tools only, the environment based SDK lookup is 
unimplemented.

Something something complexity something.

This is part of why I structured this the way as I have.  There are two parts 
of the lookup here, the part dealing with the SDK and the part dealing with the 
VC++ tools.  Now, it could be argued that this is finer point of detail that 
most will not care about, and I am concerned that is very much true, but the 
reality is that there is a bunch of complexity in the lookup and the value in 
documenting the behaviour is that we can easily lookup how to control the 
behaviour of the driver.

> I think this would be the most common case for people building from the 
> command line, and I'd suggest perhaps starting the list with this one (I 
> realize they're currently sorted by precedence, but sorting by most common 
> first would also be valid I think).

I think that common behaviour should be listed in prose.  If someone has more 
than one mechanism in the command line, the resulting behaviour would not match 
the written one and that can lead to confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146165

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

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Add some documentation on the flags and the process by which clang
identifies the headers and libraries for the Windows environment.  It
should identify the flags and their interactions as well as the order in
which the various sources of information are consulted.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146165

Files:
  clang/docs/UsersManual.rst

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -4487,3 +4487,99 @@
 to produce DLLs and EXEs. To link a DLL, pass
 ``clang_rt.asan_dll_thunk-x86_64.lib``. To link an EXE, pass
 ``-wholearchive:clang_rt.asan-x86_64.lib``.
+
+Windows System Headers and Library Lookup
+^
+
+clang-cl uses a set of different approaches to locate the right system libraries
+to link against when building code.  The Windows environment uses libraries from
+three distinct sources:
+
+1. Windows SDK
+2. UCRT (Universal C Runtime)
+3. Visual C++ Tools (VCRuntime)
+
+The Windows SDK provides the import libraries and headers required to build
+programs against the Windows system packages.  Underlying the Windows SDK is the
+UCRT, the universal C runtime.
+
+There are various controls that allow the user control over where clang-cl will
+locate these headers.  The default behaviour for the Windows SDK and UCRT is as
+follows:
+
+1. Consult the command line.
+
+Anything the user specifies is always given precedence.  The following
+extensions are part of the clang-cl toolset:
+
+- `/winsysroot:`
+
+The `/winsysroot:` is used as an equivalent to `-sysroot` on Unix
+environments.  It allows the control of an alternate location to be treated
+as a system root.  When specified, it will be used as the root where the
+`Windows Kits` is located.
+
+- `/winsdkversion:`
+- `/winsdkdir:`
+
+If `/winsysroot:` is not specified, the `/winsdkdir:` argument is consulted
+as a location to identify where the Windows SDK is located.  Contrary to
+`/winsysroot:`, `/winsdkdir:` is expected to be the complete path rather
+than a root to locate `Windows Kits`.
+
+The `/winsdkversion:` flag allows the user to specify a version identifier
+for the SDK to prefer.  When this is specified, no additional validation is
+performed and this version is preferred.  If the version is not specified,
+the highest detected version number will be used.
+
+2. Consult the environment.
+
+TODO: This is not yet implemented.
+
+This will consult the environment variables:
+- `WindowsSdkDir`
+- `UCRTVersion`
+
+3. Fallback to the registry.
+
+If no arguments are used to indicate where the SDK is present, and the
+compiler is running on Windows, the registry is consulted to locate the
+installation.
+
+The Visual C++ Toolset has a slightly more elaborate mechanism for detection.
+
+1. Consult the command line.
+
+- `/winsysroot:`
+
+The `/winsysroot:` is used as an equivalent to `-sysroot` on Unix
+environments.  It allows the control of an alternate location to be treated
+as a system root.  When specified, it will be used as the root where the
+`VC` directory is located.
+
+- `/vctoolsdir:`
+- `/vctoolsversion:`
+
+If `/winsysroot:` is not specified, the `/vctoolsdir:` argument is consulted
+as a location to identify where the Visual C++ Tools are located.  If
+`/vctoolsversion:` is specified, that version is preferred, otherwise, the
+highest version detected is used.
+
+2. Consult the environment.
+
+This will consult the environment variables:
+- `VCToolsInstallDir
+- `VCINSTALLDIR`
+- `Path`
+
+3. Consult `ISetupConfiguration` [Windows Only]
+
+Assuming that the toolchain is built with `USE_MSVC_SETUP_API` defined and
+is running on Windows, the Visual Studio COM interface `ISetupConfiguration`
+will be used to locate the installation of the MSVC toolset.
+
+4. Fallback to the registry [DEPRECATED]
+
+The registry information is used to help locate the installation as a final
+fallback.  This is only possible for pre-VS2017 installations and is
+considered deprecated.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145517: MSVC: support version preference with search

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGaf5f46822847: MSVC: support version preference with search 
(authored by compnerd).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145517

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  lld/COFF/Driver.cpp
  llvm/include/llvm/WindowsDriver/MSVCPaths.h
  llvm/lib/ExecutionEngine/Orc/COFFVCRuntimeSupport.cpp
  llvm/lib/WindowsDriver/MSVCPaths.cpp

Index: llvm/lib/WindowsDriver/MSVCPaths.cpp
===
--- llvm/lib/WindowsDriver/MSVCPaths.cpp
+++ llvm/lib/WindowsDriver/MSVCPaths.cpp
@@ -609,8 +609,9 @@
   return false;
 }
 
-bool findVCToolChainViaSetupConfig(vfs::FileSystem &VFS, std::string &Path,
-   ToolsetLayout &VSLayout) {
+bool findVCToolChainViaSetupConfig(vfs::FileSystem &VFS,
+   std::optional VCToolsVersion,
+   std::string &Path, ToolsetLayout &VSLayout) {
 #if !defined(USE_MSVC_SETUP_API)
   return false;
 #else
@@ -677,17 +678,24 @@
   std::string VCRootPath;
   convertWideToUTF8(std::wstring(VCPathWide), VCRootPath);
 
-  SmallString<256> ToolsVersionFilePath(VCRootPath);
-  sys::path::append(ToolsVersionFilePath, "Auxiliary", "Build",
-"Microsoft.VCToolsVersion.default.txt");
+  std::string ToolsVersion;
+  if (VCToolsVersion.has_value()) {
+ToolsVersion = *VCToolsVersion;
+  } else {
+SmallString<256> ToolsVersionFilePath(VCRootPath);
+sys::path::append(ToolsVersionFilePath, "Auxiliary", "Build",
+  "Microsoft.VCToolsVersion.default.txt");
+
+auto ToolsVersionFile = MemoryBuffer::getFile(ToolsVersionFilePath);
+if (!ToolsVersionFile)
+  return false;
+
+ToolsVersion = ToolsVersionFile->get()->getBuffer().rtrim();
+  }
 
-  auto ToolsVersionFile = MemoryBuffer::getFile(ToolsVersionFilePath);
-  if (!ToolsVersionFile)
-return false;
 
   SmallString<256> ToolchainPath(VCRootPath);
-  sys::path::append(ToolchainPath, "Tools", "MSVC",
-ToolsVersionFile->get()->getBuffer().rtrim());
+  sys::path::append(ToolchainPath, "Tools", "MSVC", ToolsVersion);
   auto Status = VFS.status(ToolchainPath);
   if (!Status || !Status->isDirectory())
 return false;
Index: llvm/lib/ExecutionEngine/Orc/COFFVCRuntimeSupport.cpp
===
--- llvm/lib/ExecutionEngine/Orc/COFFVCRuntimeSupport.cpp
+++ llvm/lib/ExecutionEngine/Orc/COFFVCRuntimeSupport.cpp
@@ -160,7 +160,7 @@
   if (!findVCToolChainViaCommandLine(*VFS, std::nullopt, std::nullopt,
  std::nullopt, VCToolChainPath, VSLayout) &&
   !findVCToolChainViaEnvironment(*VFS, VCToolChainPath, VSLayout) &&
-  !findVCToolChainViaSetupConfig(*VFS, VCToolChainPath, VSLayout) &&
+  !findVCToolChainViaSetupConfig(*VFS, {}, VCToolChainPath, VSLayout) &&
   !findVCToolChainViaRegistry(VCToolChainPath, VSLayout))
 return make_error("Couldn't find msvc toolchain.",
inconvertibleErrorCode());
Index: llvm/include/llvm/WindowsDriver/MSVCPaths.h
===
--- llvm/include/llvm/WindowsDriver/MSVCPaths.h
+++ llvm/include/llvm/WindowsDriver/MSVCPaths.h
@@ -90,11 +90,15 @@
ToolsetLayout &VSLayout);
 
 // Query the Setup Config server for installs, then pick the newest version
-// and find its default VC toolchain.
+// and find its default VC toolchain. If `VCToolsVersion` is specified, that
+// version is preferred over the latest version.
+//
 // This is the preferred way to discover new Visual Studios, as they're no
 // longer listed in the registry.
-bool findVCToolChainViaSetupConfig(vfs::FileSystem &VFS, std::string &Path,
-   ToolsetLayout &VSLayout);
+bool
+findVCToolChainViaSetupConfig(vfs::FileSystem &VFS,
+  std::optional VCToolsVersion,
+  std::string &Path, ToolsetLayout &VSLayout);
 
 // Look in the registry for Visual Studio installs, and use that to get
 // a toolchain path. VS2017 and newer don't get added to the registry.
Index: lld/COFF/Driver.cpp
===
--- lld/COFF/Driver.cpp
+++ lld/COFF/Driver.cpp
@@ -559,7 +559,7 @@
  WinSysRoot, vcToolChainPath, vsLayout) &&
   (Args.hasArg(OPT_lldignoreenv) ||
!findVCToolChainViaEnvironment(*VFS, vcToolChainPath, vsLayout)) &&
-  !findVCToolChainViaSetupConfig(*VFS, vcToolChainPath, vsLayout) &&
+  !findVCToolChainViaSetupConfig(*VFS, {}, vcToolChainPath, vsLayout) &&
   !findVCToolChainViaRegistry(vcToolC

[PATCH] D145517: MSVC: support version preference with search

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Sure thing, I can try to write up some details about that @hans!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145517

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

[PATCH] D145517: MSVC: support version preference with search

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added reviewers: mstorsjo, rnk.
Herald added a subscriber: hiraditya.
Herald added a project: All.
compnerd requested review of this revision.
Herald added subscribers: llvm-commits, MaskRay.
Herald added projects: clang, LLVM.

Extend the logic for the WinSDK and UCRT handling to prefer a user
specified version of the VisualC++ tools and Windows SDK.  This allows
us to now perform the regular search for the installation but select the
exact version of the SDK or VC++ tools to override the latest version.
Similar to the other flags controlling this behaviour, if the user
specifies a value, we will not perform validation on the input and will
attempt to prefer that, particularly in the case of VisualC++ tools
where no fallback occurs.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145517

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  lld/COFF/Driver.cpp
  llvm/include/llvm/WindowsDriver/MSVCPaths.h
  llvm/lib/ExecutionEngine/Orc/COFFVCRuntimeSupport.cpp
  llvm/lib/WindowsDriver/MSVCPaths.cpp

Index: llvm/lib/WindowsDriver/MSVCPaths.cpp
===
--- llvm/lib/WindowsDriver/MSVCPaths.cpp
+++ llvm/lib/WindowsDriver/MSVCPaths.cpp
@@ -609,8 +609,9 @@
   return false;
 }
 
-bool findVCToolChainViaSetupConfig(vfs::FileSystem &VFS, std::string &Path,
-   ToolsetLayout &VSLayout) {
+bool findVCToolChainViaSetupConfig(vfs::FileSystem &VFS,
+   std::optional VCToolsVersion,
+   std::string &Path, ToolsetLayout &VSLayout) {
 #if !defined(USE_MSVC_SETUP_API)
   return false;
 #else
@@ -677,17 +678,24 @@
   std::string VCRootPath;
   convertWideToUTF8(std::wstring(VCPathWide), VCRootPath);
 
-  SmallString<256> ToolsVersionFilePath(VCRootPath);
-  sys::path::append(ToolsVersionFilePath, "Auxiliary", "Build",
-"Microsoft.VCToolsVersion.default.txt");
+  std::string ToolsVersion;
+  if (VCToolsVersion.has_value()) {
+ToolsVersion = *VCToolsVersion;
+  } else {
+SmallString<256> ToolsVersionFilePath(VCRootPath);
+sys::path::append(ToolsVersionFilePath, "Auxiliary", "Build",
+  "Microsoft.VCToolsVersion.default.txt");
+
+auto ToolsVersionFile = MemoryBuffer::getFile(ToolsVersionFilePath);
+if (!ToolsVersionFile)
+  return false;
+
+ToolsVersion = ToolsVersionFile->get()->getBuffer().rtrim();
+  }
 
-  auto ToolsVersionFile = MemoryBuffer::getFile(ToolsVersionFilePath);
-  if (!ToolsVersionFile)
-return false;
 
   SmallString<256> ToolchainPath(VCRootPath);
-  sys::path::append(ToolchainPath, "Tools", "MSVC",
-ToolsVersionFile->get()->getBuffer().rtrim());
+  sys::path::append(ToolchainPath, "Tools", "MSVC", ToolsVersion);
   auto Status = VFS.status(ToolchainPath);
   if (!Status || !Status->isDirectory())
 return false;
Index: llvm/lib/ExecutionEngine/Orc/COFFVCRuntimeSupport.cpp
===
--- llvm/lib/ExecutionEngine/Orc/COFFVCRuntimeSupport.cpp
+++ llvm/lib/ExecutionEngine/Orc/COFFVCRuntimeSupport.cpp
@@ -160,7 +160,7 @@
   if (!findVCToolChainViaCommandLine(*VFS, std::nullopt, std::nullopt,
  std::nullopt, VCToolChainPath, VSLayout) &&
   !findVCToolChainViaEnvironment(*VFS, VCToolChainPath, VSLayout) &&
-  !findVCToolChainViaSetupConfig(*VFS, VCToolChainPath, VSLayout) &&
+  !findVCToolChainViaSetupConfig(*VFS, {}, VCToolChainPath, VSLayout) &&
   !findVCToolChainViaRegistry(VCToolChainPath, VSLayout))
 return make_error("Couldn't find msvc toolchain.",
inconvertibleErrorCode());
Index: llvm/include/llvm/WindowsDriver/MSVCPaths.h
===
--- llvm/include/llvm/WindowsDriver/MSVCPaths.h
+++ llvm/include/llvm/WindowsDriver/MSVCPaths.h
@@ -90,11 +90,15 @@
ToolsetLayout &VSLayout);
 
 // Query the Setup Config server for installs, then pick the newest version
-// and find its default VC toolchain.
+// and find its default VC toolchain. If `VCToolsVersion` is specified, that
+// version is preferred over the latest version.
+//
 // This is the preferred way to discover new Visual Studios, as they're no
 // longer listed in the registry.
-bool findVCToolChainViaSetupConfig(vfs::FileSystem &VFS, std::string &Path,
-   ToolsetLayout &VSLayout);
+bool
+findVCToolChainViaSetupConfig(vfs::FileSystem &VFS,
+  std::optional VCToolsVersion,
+  std::string &Path, ToolsetLayout &VSLayout);
 
 // Look in the registry for Visual Studio installs, and use that to get
 // a toolchain path. VS2017 and newer don't get added to the registry.
Index: lld/COFF/Driver.cpp
=

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf1440bf6fd22: Driver: introduce GNU spellings to control 
MSVC paths (authored by compnerd).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145007

Files:
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -7044,6 +7044,16 @@
 def _SLASH_Gregcall : CLFlag<"Gregcall">,
   HelpText<"Set __regcall as a default calling convention">;
 
+// GNU Driver aliases
+
+def : Separate<["-"], "Xmicrosoft-visualc-tools-root">, 
Alias<_SLASH_vctoolsdir>;
+def : Separate<["-"], "Xmicrosoft-visualc-tools-version">,
+Alias<_SLASH_vctoolsversion>;
+def : Separate<["-"], "Xmicrosoft-windows-sdk-root">,
+Alias<_SLASH_winsdkdir>;
+def : Separate<["-"], "Xmicrosoft-windows-sdk-version">,
+Alias<_SLASH_winsdkversion>;
+
 // Ignored:
 
 def _SLASH_analyze_ : CLIgnoredFlag<"analyze-">;


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -7044,6 +7044,16 @@
 def _SLASH_Gregcall : CLFlag<"Gregcall">,
   HelpText<"Set __regcall as a default calling convention">;
 
+// GNU Driver aliases
+
+def : Separate<["-"], "Xmicrosoft-visualc-tools-root">, Alias<_SLASH_vctoolsdir>;
+def : Separate<["-"], "Xmicrosoft-visualc-tools-version">,
+Alias<_SLASH_vctoolsversion>;
+def : Separate<["-"], "Xmicrosoft-windows-sdk-root">,
+Alias<_SLASH_winsdkdir>;
+def : Separate<["-"], "Xmicrosoft-windows-sdk-version">,
+Alias<_SLASH_winsdkversion>;
+
 // Ignored:
 
 def _SLASH_analyze_ : CLIgnoredFlag<"analyze-">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Just a friendly reminder, I'd like to get this merged to do a follow up change 
to improve some of the usability of these flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145007

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

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Yeah, I suspect that would be difficult.  Additionally, these are aliases, so 
they should already have testing coverage through the existing tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145007

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

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added reviewers: rnk, mstorsjo.
Herald added a project: All.
compnerd requested review of this revision.
Herald added a project: clang.

Add a set of `-Xmicrosoft` flags to control the Windows SDK and VisualC
tools directories.  This allows control over the selection of the SDK
and tools when using the GNU driver.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145007

Files:
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -7044,6 +7044,16 @@
 def _SLASH_Gregcall : CLFlag<"Gregcall">,
   HelpText<"Set __regcall as a default calling convention">;
 
+// GNU Driver aliases
+
+def : Separate<["-"], "Xmicrosoft-visualc-tools-root">, 
Alias<_SLASH_vctoolsdir>;
+def : Separate<["-"], "Xmicrosoft-visualc-tools-version">,
+Alias<_SLASH_vctoolsversion>;
+def : Separate<["-"], "Xmicrosoft-windows-sdk-root">,
+Alias<_SLASH_winsdkdir>;
+def : Separate<["-"], "Xmicrosoft-windows-sdk-version">,
+Alias<_SLASH_winsdkversion>;
+
 // Ignored:
 
 def _SLASH_analyze_ : CLIgnoredFlag<"analyze-">;


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -7044,6 +7044,16 @@
 def _SLASH_Gregcall : CLFlag<"Gregcall">,
   HelpText<"Set __regcall as a default calling convention">;
 
+// GNU Driver aliases
+
+def : Separate<["-"], "Xmicrosoft-visualc-tools-root">, Alias<_SLASH_vctoolsdir>;
+def : Separate<["-"], "Xmicrosoft-visualc-tools-version">,
+Alias<_SLASH_vctoolsversion>;
+def : Separate<["-"], "Xmicrosoft-windows-sdk-root">,
+Alias<_SLASH_winsdkdir>;
+def : Separate<["-"], "Xmicrosoft-windows-sdk-version">,
+Alias<_SLASH_winsdkversion>;
+
 // Ignored:
 
 def _SLASH_analyze_ : CLIgnoredFlag<"analyze-">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140507: Parse: handle another case of invalid handling for attributes

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG179d24d764ac: Parse: handle another case of invalid handling 
for attributes (authored by compnerd).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140507

Files:
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/cxx-attributes.cpp


Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -3,6 +3,8 @@
 // GH#58229 - rejects-valid
 __attribute__((__visibility__("default"))) [[nodiscard]] int f();
 [[nodiscard]] __attribute__((__visibility__("default"))) int f();
+extern "C" __attribute__((__visibility__("default"))) [[nodiscard]]
+int g() { return 32; }
 
 class c {
   virtual void f1(const char* a, ...)
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -359,8 +359,11 @@
 Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+ MaybeParseGNUAttributes(DeclSpecAttrs))
+;
 
   if (Tok.isNot(tok::l_brace)) {
 // Reset the source range in DS, as the leading "extern"
@@ -369,7 +372,7 @@
 DS.SetRangeEnd(SourceLocation());
 // ... but anyway remember that such an "extern" was seen.
 DS.setExternInLinkageSpec(true);
-ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs, &DS);
+ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs, &DS);
 return LinkageSpec ? Actions.ActOnFinishLinkageSpecification(
  getCurScope(), LinkageSpec, SourceLocation())
: nullptr;
@@ -411,7 +414,7 @@
 default:
   ParsedAttributes DeclAttrs(AttrFactory);
   MaybeParseCXX11Attributes(DeclAttrs);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
   continue;
 }
 


Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -3,6 +3,8 @@
 // GH#58229 - rejects-valid
 __attribute__((__visibility__("default"))) [[nodiscard]] int f();
 [[nodiscard]] __attribute__((__visibility__("default"))) int f();
+extern "C" __attribute__((__visibility__("default"))) [[nodiscard]]
+int g() { return 32; }
 
 class c {
   virtual void f1(const char* a, ...)
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -359,8 +359,11 @@
 Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+ MaybeParseGNUAttributes(DeclSpecAttrs))
+;
 
   if (Tok.isNot(tok::l_brace)) {
 // Reset the source range in DS, as the leading "extern"
@@ -369,7 +372,7 @@
 DS.SetRangeEnd(SourceLocation());
 // ... but anyway remember that such an "extern" was seen.
 DS.setExternInLinkageSpec(true);
-ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs, &DS);
+ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs, &DS);
 return LinkageSpec ? Actions.ActOnFinishLinkageSpecification(
  getCurScope(), LinkageSpec, SourceLocation())
: nullptr;
@@ -411,7 +414,7 @@
 default:
   ParsedAttributes DeclAttrs(AttrFactory);
   MaybeParseCXX11Attributes(DeclAttrs);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
   continue;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140507: Parse: handle another case of invalid handling for attributes

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

I think that the current release note should be sufficient.  This is handling 
the same scenario in a different path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140507

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

[PATCH] D140507: Parse: handle another case of invalid handling for attributes

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added a reviewer: aaron.ballman.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
compnerd requested review of this revision.
Herald added a project: clang.

clang would improperly disallow GNU attributes before C++ standard
attributes when a declaration had a linkage specifier.  Handle this
similarly to the previous case of invalid parsing.  We now better match
the parsing rules from GCC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140507

Files:
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/cxx-attributes.cpp


Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -3,6 +3,8 @@
 // GH#58229 - rejects-valid
 __attribute__((__visibility__("default"))) [[nodiscard]] int f();
 [[nodiscard]] __attribute__((__visibility__("default"))) int f();
+extern "C" __attribute__((__visibility__("default"))) [[nodiscard]]
+int g() { return 32; }
 
 class c {
   virtual void f1(const char* a, ...)
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -359,8 +359,11 @@
 Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+ MaybeParseGNUAttributes(DeclSpecAttrs))
+;
 
   if (Tok.isNot(tok::l_brace)) {
 // Reset the source range in DS, as the leading "extern"
@@ -369,7 +372,7 @@
 DS.SetRangeEnd(SourceLocation());
 // ... but anyway remember that such an "extern" was seen.
 DS.setExternInLinkageSpec(true);
-ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs, &DS);
+ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs, &DS);
 return LinkageSpec ? Actions.ActOnFinishLinkageSpecification(
  getCurScope(), LinkageSpec, SourceLocation())
: nullptr;
@@ -411,7 +414,7 @@
 default:
   ParsedAttributes DeclAttrs(AttrFactory);
   MaybeParseCXX11Attributes(DeclAttrs);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
   continue;
 }
 


Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -3,6 +3,8 @@
 // GH#58229 - rejects-valid
 __attribute__((__visibility__("default"))) [[nodiscard]] int f();
 [[nodiscard]] __attribute__((__visibility__("default"))) int f();
+extern "C" __attribute__((__visibility__("default"))) [[nodiscard]]
+int g() { return 32; }
 
 class c {
   virtual void f1(const char* a, ...)
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -359,8 +359,11 @@
 Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+ MaybeParseGNUAttributes(DeclSpecAttrs))
+;
 
   if (Tok.isNot(tok::l_brace)) {
 // Reset the source range in DS, as the leading "extern"
@@ -369,7 +372,7 @@
 DS.SetRangeEnd(SourceLocation());
 // ... but anyway remember that such an "extern" was seen.
 DS.setExternInLinkageSpec(true);
-ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs, &DS);
+ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs, &DS);
 return LinkageSpec ? Actions.ActOnFinishLinkageSpecification(
  getCurScope(), LinkageSpec, SourceLocation())
: nullptr;
@@ -411,7 +414,7 @@
 default:
   ParsedAttributes DeclAttrs(AttrFactory);
   MaybeParseCXX11Attributes(DeclAttrs);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
   continue;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139266: Headers: tweak inclusion condition for stdatomic.h

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

@thakis and https://github.com/llvm/llvm-project/issues/59640


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139266

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

[PATCH] D139266: Headers: tweak inclusion condition for stdatomic.h

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe0c3142af075: Headers: tweak inclusion condition for 
stdatomic.h (authored by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D139266?vs=480531&id=483327#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139266

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Headers/stdatomic.h


Index: clang/lib/Headers/stdatomic.h
===
--- clang/lib/Headers/stdatomic.h
+++ clang/lib/Headers/stdatomic.h
@@ -15,10 +15,12 @@
  *
  * Exclude the MSVC path as well as the MSVC header as of the 14.31.30818
  * explicitly disallows `stdatomic.h` in the C mode via an `#error`.  Fallback
- * to the clang resource header until that is fully supported.
+ * to the clang resource header until that is fully supported.  The
+ * `stdatomic.h` header requires C++ 23 or newer.
  */
 #if __STDC_HOSTED__ && 
\
-__has_include_next() && !(defined(_MSC_VER) && 
!defined(__cplusplus))
+__has_include_next() &&   
\
+!(defined(_MSC_VER) && defined(__cplusplus) && __cplusplus < 202002L)
 # include_next 
 #else
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -199,6 +199,8 @@
 
 Bug Fixes
 -
+- ``stdatomic.h`` will use the internal declarations when targeting pre-C++-23
+  on Windows platforms as the MSVC support requires newer C++ standard.
 - Correct ``_Static_assert`` to accept the same set of extended integer
   constant expressions as is accpted in other contexts that accept them.
   This fixes `Issue 57687 
`_.


Index: clang/lib/Headers/stdatomic.h
===
--- clang/lib/Headers/stdatomic.h
+++ clang/lib/Headers/stdatomic.h
@@ -15,10 +15,12 @@
  *
  * Exclude the MSVC path as well as the MSVC header as of the 14.31.30818
  * explicitly disallows `stdatomic.h` in the C mode via an `#error`.  Fallback
- * to the clang resource header until that is fully supported.
+ * to the clang resource header until that is fully supported.  The
+ * `stdatomic.h` header requires C++ 23 or newer.
  */
 #if __STDC_HOSTED__ && \
-__has_include_next() && !(defined(_MSC_VER) && !defined(__cplusplus))
+__has_include_next() &&   \
+!(defined(_MSC_VER) && defined(__cplusplus) && __cplusplus < 202002L)
 # include_next 
 #else
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -199,6 +199,8 @@
 
 Bug Fixes
 -
+- ``stdatomic.h`` will use the internal declarations when targeting pre-C++-23
+  on Windows platforms as the MSVC support requires newer C++ standard.
 - Correct ``_Static_assert`` to accept the same set of extended integer
   constant expressions as is accpted in other contexts that accept them.
   This fixes `Issue 57687 `_.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139749: Headers: use C++ inline semantics in C++ mode

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Thanks @fsb4000 (and @CaseyCarter)!  I think that due to the shipped version, 
it makes sense to do this still.  Using standard semantics in general I think 
is preferable, since nothing prevents another compiler implementation to still 
do something similar.


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

https://reviews.llvm.org/D139749

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

[PATCH] D139749: Headers: use C++ inline semantics in C++ mode

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 483081.
compnerd retitled this revision from "Headers: make a couple of builtins 
non-static" to "Headers: use C++ inline semantics in C++ mode".
compnerd edited the summary of this revision.

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

https://reviews.llvm.org/D139749

Files:
  clang/lib/Headers/adxintrin.h


Index: clang/lib/Headers/adxintrin.h
===
--- clang/lib/Headers/adxintrin.h
+++ clang/lib/Headers/adxintrin.h
@@ -17,8 +17,19 @@
 /* Define the default attributes for the functions in this file. */
 #define __DEFAULT_FN_ATTRS __attribute__((__always_inline__, __nodebug__))
 
+/* Use C++ inline semantics in C++, GNU inline for C mode. */
+#if defined(__cplusplus)
+#define __INLINE __inline
+#else
+#define __INLINE static __inline
+#endif
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
 /* Intrinsics that are available only if __ADX__ defined */
-static __inline unsigned char __attribute__((__always_inline__, __nodebug__, 
__target__("adx")))
+__INLINE unsigned char __attribute__((__always_inline__, __nodebug__, 
__target__("adx")))
 _addcarryx_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
unsigned int *__p)
 {
@@ -26,7 +37,7 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __attribute__((__always_inline__, __nodebug__, 
__target__("adx")))
+__INLINE unsigned char __attribute__((__always_inline__, __nodebug__, 
__target__("adx")))
 _addcarryx_u64(unsigned char __cf, unsigned long long __x,
unsigned long long __y, unsigned long long  *__p)
 {
@@ -35,7 +46,7 @@
 #endif
 
 /* Intrinsics that are also available if __ADX__ undefined */
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _addcarry_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
   unsigned int *__p)
 {
@@ -43,7 +54,7 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _addcarry_u64(unsigned char __cf, unsigned long long __x,
   unsigned long long __y, unsigned long long  *__p)
 {
@@ -51,7 +62,7 @@
 }
 #endif
 
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _subborrow_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
   unsigned int *__p)
 {
@@ -59,7 +70,7 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _subborrow_u64(unsigned char __cf, unsigned long long __x,
unsigned long long __y, unsigned long long  *__p)
 {
@@ -67,6 +78,11 @@
 }
 #endif
 
+#if defined(__cplusplus)
+}
+#endif
+
 #undef __DEFAULT_FN_ATTRS
+#undef __INLINE
 
 #endif /* __ADXINTRIN_H */


Index: clang/lib/Headers/adxintrin.h
===
--- clang/lib/Headers/adxintrin.h
+++ clang/lib/Headers/adxintrin.h
@@ -17,8 +17,19 @@
 /* Define the default attributes for the functions in this file. */
 #define __DEFAULT_FN_ATTRS __attribute__((__always_inline__, __nodebug__))
 
+/* Use C++ inline semantics in C++, GNU inline for C mode. */
+#if defined(__cplusplus)
+#define __INLINE __inline
+#else
+#define __INLINE static __inline
+#endif
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
 /* Intrinsics that are available only if __ADX__ defined */
-static __inline unsigned char __attribute__((__always_inline__, __nodebug__, __target__("adx")))
+__INLINE unsigned char __attribute__((__always_inline__, __nodebug__, __target__("adx")))
 _addcarryx_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
unsigned int *__p)
 {
@@ -26,7 +37,7 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __attribute__((__always_inline__, __nodebug__, __target__("adx")))
+__INLINE unsigned char __attribute__((__always_inline__, __nodebug__, __target__("adx")))
 _addcarryx_u64(unsigned char __cf, unsigned long long __x,
unsigned long long __y, unsigned long long  *__p)
 {
@@ -35,7 +46,7 @@
 #endif
 
 /* Intrinsics that are also available if __ADX__ undefined */
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _addcarry_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
   unsigned int *__p)
 {
@@ -43,7 +54,7 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _addcarry_u64(unsigned char __cf, unsigned long long __x,
   unsigned long long __y, unsigned long long  *__p)
 {
@@ -51,7 +62,7 @@
 }
 #endif
 
-static __inline unsigned char __DEFAULT_FN_ATTRS
+__INLINE unsigned char __DEFAULT_FN_ATTRS
 _subborrow_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
   unsigned int *__p)
 {
@@ -59,7 +70,7 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_F

[PATCH] D139749: Headers: make a couple of builtins non-static

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Seems that this alone is insufficient as some build does run into issues, so I 
may need to refine this further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139749

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

[PATCH] D139749: Headers: make a couple of builtins non-static

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a subscriber: STL_MSFT.
compnerd added a comment.

@fsb4000 is my reading correct that MSVC will look into trying to handle 
`static inline` even though it is a GNUism?  I wonder if we should consider 
limiting the use of `static inline` to C mode rather than including C++.  I 
also wonder if I can loosen it similarly to avoid the issue I'm running into 
(which @STL_MSFT correctly identified - modules).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139749

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

[PATCH] D139749: Headers: make a couple of builtins non-static

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added a reviewer: aaron.ballman.
Herald added a project: All.
compnerd requested review of this revision.
Herald added a project: clang.

When building with the 17.5.0 preview toolset for MSVC and building with 
modules, the definition of `_addcarry_u64` and `_subborrow_u64` seem to cause 
issues due to the `static` definition following the non-static declaration in 
the MSVC headers.  Elide the `static` storage class on the declaration in MSVC 
mode.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139749

Files:
  clang/lib/Headers/adxintrin.h


Index: clang/lib/Headers/adxintrin.h
===
--- clang/lib/Headers/adxintrin.h
+++ clang/lib/Headers/adxintrin.h
@@ -43,7 +43,10 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+#if !defined(_MSC_VER)
+static
+#endif
+__inline unsigned char __DEFAULT_FN_ATTRS
 _addcarry_u64(unsigned char __cf, unsigned long long __x,
   unsigned long long __y, unsigned long long  *__p)
 {
@@ -59,7 +62,10 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+#if !defined(_MSC_VER)
+static
+#endif
+__inline unsigned char __DEFAULT_FN_ATTRS
 _subborrow_u64(unsigned char __cf, unsigned long long __x,
unsigned long long __y, unsigned long long  *__p)
 {


Index: clang/lib/Headers/adxintrin.h
===
--- clang/lib/Headers/adxintrin.h
+++ clang/lib/Headers/adxintrin.h
@@ -43,7 +43,10 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+#if !defined(_MSC_VER)
+static
+#endif
+__inline unsigned char __DEFAULT_FN_ATTRS
 _addcarry_u64(unsigned char __cf, unsigned long long __x,
   unsigned long long __y, unsigned long long  *__p)
 {
@@ -59,7 +62,10 @@
 }
 
 #ifdef __x86_64__
-static __inline unsigned char __DEFAULT_FN_ATTRS
+#if !defined(_MSC_VER)
+static
+#endif
+__inline unsigned char __DEFAULT_FN_ATTRS
 _subborrow_u64(unsigned char __cf, unsigned long long __x,
unsigned long long __y, unsigned long long  *__p)
 {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137337: Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: bolt/lib/RuntimeLibs/RuntimeLibrary.cpp:32
   SmallString<128> LibPath = llvm::sys::path::parent_path(Dir);
-  llvm::sys::path::append(LibPath, "lib" LLVM_LIBDIR_SUFFIX);
+  llvm::sys::path::append(LibPath, CMAKE_INSTALL_LIBDIR);
   if (!llvm::sys::fs::exists(LibPath)) {

Ericson2314 wrote:
> compnerd wrote:
> > serge-sans-paille wrote:
> > > Ericson2314 wrote:
> > > > mgorny wrote:
> > > > > Well, one thing I immediately notice is that technically 
> > > > > `CMAKE_INSTALL_LIBDIR` can be an absolute path, while in many 
> > > > > locations you are assuming it will always be relative. Not saying 
> > > > > that's a blocker but I think you should add checks for that into 
> > > > > `CMakeLists.txt`.
> > > > Yes I was working on this, and I intend to use `CMAKE_INSTALL_LIBDIR` 
> > > > with an absolute path.
> > > > 
> > > > OK if this isn't supported initially but we should ovoid regressing 
> > > > where possible.
> > > Turns out LLVM_LIBDIR_SUFFIX is used in several situations: (a) for the 
> > > library install dir or (b) for the location where build artifact as 
> > > stored in CMAKE_BINARY_DIR. (a) could accept a path but not (b), unless 
> > > we derive it from (a) but I can see a lot of complication there for the 
> > > testing step. Would that be ok to choke on CMAKE_INSTALL_LIBDIR being a 
> > > path and not a directory component?
> > > 
> > I think that is absolutely reasonable.  There is the 
> > `CMAKE_INSTALL_FULL_LIBDIR` which should be the relatively absolute path 
> > (it is relative to `DESTDIR`).  The `CMAKE_INSTALL_LIBDIR` should be the 
> > relative component which is added to `CMAKE_INSTALL_PREFIX`.
> Please do not do this. In NixOS we uses absolute paths for these which are 
> *not* within `CMAKE_INSTALL_PREFIX` and I would very much like that to 
> continue to work, and have put a lot of effort into it.
> 
> (I am sorry I have been a bit AWAL on LLVM things in general but hopefully 
> will have more time as we head into the new year.)
Why can NixOS not use `CMAKE_INSTALL_FULL_LIBDIR`?  That is computed to 
`${CMAKE_INSTALL_PREFIX}${CMAKE_INSTALL_LIBDIR}` only if it is not already 
defined to an absolute path.


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

https://reviews.llvm.org/D137337

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

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

(Accepting Revision for Closing)
I missed the trailing number of the differential revision, and it failed to tie 
it to this.  The commit is at  
https://github.com/llvm/llvm-project/commit/707cc06e1570b5966efcd6a9124191c80fa7a754
@rnk is going to be out for a while, so any concerns can be addressed in a 
follow up.


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

https://reviews.llvm.org/D138514

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

[PATCH] D137337: Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: bolt/lib/RuntimeLibs/RuntimeLibrary.cpp:32
   SmallString<128> LibPath = llvm::sys::path::parent_path(Dir);
-  llvm::sys::path::append(LibPath, "lib" LLVM_LIBDIR_SUFFIX);
+  llvm::sys::path::append(LibPath, CMAKE_INSTALL_LIBDIR);
   if (!llvm::sys::fs::exists(LibPath)) {

serge-sans-paille wrote:
> Ericson2314 wrote:
> > mgorny wrote:
> > > Well, one thing I immediately notice is that technically 
> > > `CMAKE_INSTALL_LIBDIR` can be an absolute path, while in many locations 
> > > you are assuming it will always be relative. Not saying that's a blocker 
> > > but I think you should add checks for that into `CMakeLists.txt`.
> > Yes I was working on this, and I intend to use `CMAKE_INSTALL_LIBDIR` with 
> > an absolute path.
> > 
> > OK if this isn't supported initially but we should ovoid regressing where 
> > possible.
> Turns out LLVM_LIBDIR_SUFFIX is used in several situations: (a) for the 
> library install dir or (b) for the location where build artifact as stored in 
> CMAKE_BINARY_DIR. (a) could accept a path but not (b), unless we derive it 
> from (a) but I can see a lot of complication there for the testing step. 
> Would that be ok to choke on CMAKE_INSTALL_LIBDIR being a path and not a 
> directory component?
> 
I think that is absolutely reasonable.  There is the 
`CMAKE_INSTALL_FULL_LIBDIR` which should be the relatively absolute path (it is 
relative to `DESTDIR`).  The `CMAKE_INSTALL_LIBDIR` should be the relative 
component which is added to `CMAKE_INSTALL_PREFIX`.


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

https://reviews.llvm.org/D137337

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

[PATCH] D139266: Headers: tweak inclusion condition for stdatomic.h

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Update to avoid use of the `-0` trick.


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

https://reviews.llvm.org/D139266

Files:
  clang/lib/Headers/stdatomic.h


Index: clang/lib/Headers/stdatomic.h
===
--- clang/lib/Headers/stdatomic.h
+++ clang/lib/Headers/stdatomic.h
@@ -15,10 +15,12 @@
  *
  * Exclude the MSVC path as well as the MSVC header as of the 14.31.30818
  * explicitly disallows `stdatomic.h` in the C mode via an `#error`.  Fallback
- * to the clang resource header until that is fully supported.
+ * to the clang resource header until that is fully supported.  The
+ * `stdatomic.h` header requires C++ 23 or newer.
  */
 #if __STDC_HOSTED__ && 
\
-__has_include_next() && !(defined(_MSC_VER) && 
!defined(__cplusplus))
+__has_include_next() &&   
\
+!(defined(_MSC_VER) && defined(__cplusplus) && __cplusplus < 202002l)
 # include_next 
 #else



Index: clang/lib/Headers/stdatomic.h
===
--- clang/lib/Headers/stdatomic.h
+++ clang/lib/Headers/stdatomic.h
@@ -15,10 +15,12 @@
  *
  * Exclude the MSVC path as well as the MSVC header as of the 14.31.30818
  * explicitly disallows `stdatomic.h` in the C mode via an `#error`.  Fallback
- * to the clang resource header until that is fully supported.
+ * to the clang resource header until that is fully supported.  The
+ * `stdatomic.h` header requires C++ 23 or newer.
  */
 #if __STDC_HOSTED__ && \
-__has_include_next() && !(defined(_MSC_VER) && !defined(__cplusplus))
+__has_include_next() &&   \
+!(defined(_MSC_VER) && defined(__cplusplus) && __cplusplus < 202002l)
 # include_next 
 #else

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

[PATCH] D139266: Headers: tweak inclusion condition for stdatomic.h

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/lib/Headers/stdatomic.h:21
+#if __STDC_HOSTED__ && __has_include_next()   
 \
+&& !(defined(_MSC_VER) && __cplusplus-0 < 202002l)
 # include_next 

Mordante wrote:
> Is `__cplusplus-0` intentional? If so please add some comments how this 
> differs from `__cplusplus`.
> Since this is a C header we should test whether the macro exists before 
> querying its value.
Yes, that is intentional.  The `-0` is a trick!  That avoids the need to check 
for the definition, because it will be evaluated to as the value of 
`__cplusplus` or be `0` due to the expansion to CPP rules and `-0`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139266

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

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Add a test case for member data.


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

https://reviews.llvm.org/D138514

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/Sema/gnu-asm-pmf.cpp


Index: clang/test/Sema/gnu-asm-pmf.cpp
===
--- /dev/null
+++ clang/test/Sema/gnu-asm-pmf.cpp
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++2b 
-fsyntax-only %s -verify
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -std=c++2b 
-fsyntax-only %s -verify
+
+struct S {
+  void operator()();
+};
+
+struct T {
+  virtual void operator()();
+};
+
+struct U {
+  static void operator()();
+};
+
+struct V: virtual T {
+  virtual void f();
+};
+
+struct W : virtual V {
+  int i;
+};
+
+struct X {
+  __UINTPTR_TYPE__ ptr;
+  __UINTPTR_TYPE__ adj;
+};
+
+auto L = [](){};
+
+void f() {
+  auto pmf = &S::operator();
+
+  __asm__ __volatile__ ("" : : "r"(&decltype(L)::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&S::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&T::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(pmf));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&W::f));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&W::i));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+
+  __asm__ __volatile__ ("" : : "r"(X{0,0}));
+  __asm__ __volatile__ ("" : : "r"(&U::operator()));
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -377,6 +377,11 @@
 
 Expr *InputExpr = Exprs[i];
 
+if (InputExpr->getType()->isMemberPointerType())
+  return StmtError(Diag(InputExpr->getBeginLoc(),
+diag::err_asm_pmf_through_constraint_not_permitted)
+   << InputExpr->getSourceRange());
+
 // Referring to parameters is not allowed in naked functions.
 if (CheckNakedParmReference(InputExpr, *this))
   return StmtError();
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8841,6 +8841,9 @@
 
 // inline asm.
 let CategoryName = "Inline Assembly Issue" in {
+  def err_asm_pmf_through_constraint_not_permitted
+: Error<"passing pointer-to-member through register constrained inline "
+"assembly parameter is not permitted">;
   def err_asm_invalid_lvalue_in_output : Error<"invalid lvalue in asm output">;
   def err_asm_invalid_output_constraint : Error<
 "invalid output constraint '%0' in asm">;


Index: clang/test/Sema/gnu-asm-pmf.cpp
===
--- /dev/null
+++ clang/test/Sema/gnu-asm-pmf.cpp
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++2b -fsyntax-only %s -verify
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -std=c++2b -fsyntax-only %s -verify
+
+struct S {
+  void operator()();
+};
+
+struct T {
+  virtual void operator()();
+};
+
+struct U {
+  static void operator()();
+};
+
+struct V: virtual T {
+  virtual void f();
+};
+
+struct W : virtual V {
+  int i;
+};
+
+struct X {
+  __UINTPTR_TYPE__ ptr;
+  __UINTPTR_TYPE__ adj;
+};
+
+auto L = [](){};
+
+void f() {
+  auto pmf = &S::operator();
+
+  __asm__ __volatile__ ("" : : "r"(&decltype(L)::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&S::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&T::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(pmf));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" 

[PATCH] D139266: Headers: tweak inclusion condition for stdatomic.h

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added reviewers: aaron.ballman, Mordante, fsb4000.
Herald added a project: All.
compnerd requested review of this revision.
Herald added a project: clang.

MSVC requires that C++23 be available (`_HAS_CXX23`) else the entire
content is elided.  Conditionalise the inclusion properly so that C/C++
code using `stdatomic.h` for `memory_order_*` constants are able to do
so without changing the C++ standard.  This repairs builds of Swift and
libdispatch after ba49d39b20cc5358da28af2ac82bd336028780bc 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139266

Files:
  clang/lib/Headers/stdatomic.h


Index: clang/lib/Headers/stdatomic.h
===
--- clang/lib/Headers/stdatomic.h
+++ clang/lib/Headers/stdatomic.h
@@ -17,8 +17,8 @@
  * explicitly disallows `stdatomic.h` in the C mode via an `#error`.  Fallback
  * to the clang resource header until that is fully supported.
  */
-#if __STDC_HOSTED__ && 
\
-__has_include_next() && !(defined(_MSC_VER) && 
!defined(__cplusplus))
+#if __STDC_HOSTED__ && __has_include_next()   
 \
+&& !(defined(_MSC_VER) && __cplusplus-0 < 202002l)
 # include_next 
 #else
 


Index: clang/lib/Headers/stdatomic.h
===
--- clang/lib/Headers/stdatomic.h
+++ clang/lib/Headers/stdatomic.h
@@ -17,8 +17,8 @@
  * explicitly disallows `stdatomic.h` in the C mode via an `#error`.  Fallback
  * to the clang resource header until that is fully supported.
  */
-#if __STDC_HOSTED__ && \
-__has_include_next() && !(defined(_MSC_VER) && !defined(__cplusplus))
+#if __STDC_HOSTED__ && __has_include_next()\
+&& !(defined(_MSC_VER) && __cplusplus-0 < 202002l)
 # include_next 
 #else
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:381
+if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
+  if (const auto *UO = dyn_cast(InputExpr))
+if (UO->getOpcode() == UO_AddrOf)

rnk wrote:
> compnerd wrote:
> > aaron.ballman wrote:
> > > rnk wrote:
> > > > This is too narrow, there are lots of other ways to do this:
> > > > ```
> > > > struct Foo { void method(); };
> > > > void f() {
> > > >   auto pmf = &Foo::method;
> > > >   asm volatile ("" : : "r"(pmf));
> > > > }
> > > > ```
> > > > 
> > > > I think it makes sense to check for:
> > > > * An expression with a member pointer type
> > > > * Where the size of the type is larger than the size of a pointer, or 
> > > > word, or whatever proxy we normally use for the size of a general 
> > > > purpose register
> > > > 
> > > > In the Microsoft ABI, member function pointers are only sometimes 
> > > > pointer-sized. If the class uses the multiple inheritance model, it 
> > > > will be bigger and include the this-adjuster field. See the inheritance 
> > > > keyword docs to learn more:
> > > > https://learn.microsoft.com/en-us/cpp/cpp/inheritance-keywords?view=msvc-170
> > > > 
> > > > This also handles large pointers to data members in the MS ABI, which 
> > > > also has a wacky aggregate representation.
> > > That sounds like a less fragile approach to me, thanks for that 
> > > suggestion!
> > Thanks @rnk for pointing out the oversight on handling a PMF through a 
> > VarDecl and the pointer to the MSDN docs on how to form the adjusted 
> > reference.  This actually is more interesting.  Consider now the following:
> > 
> > ```
> > struct s {
> > __UINTPTR_TYPE__ i, j;
> > };
> > 
> > void f() {
> > __asm__ __volatile__("" : : "r"(s{0,0}));
> > }
> > 
> > struct u { virtual void f(); };
> > struct v { virtual void operator()(); };
> > struct w: u, v {
> > };
> > 
> > void g() {
> > __asm__ __volatile__("" : : "r"(&w::operator()));
> > }
> > ```
> > 
> > GCC does accept the input, but clang fails due to the backend failing to 
> > form the indirect passing for the aggregate (`Don't know how to handle 
> > indirect register inputs yet for constraint 'r'`).  Interestingly enough, 
> > when targeting `x86_64-unknown-windows-msvc` instead of 
> > `x86_64-unknown-linux-gnu`, we represent the PMF as an aggregate and 
> > trigger the same type of aggregate passing failure.  On Linux though we 
> > attempt to lower the aggregate passing and then fail to select the copy due 
> > to the aggregate being lowered into a single register rather than being 
> > passed as indirect.  One of the issues with the indirect passing is that 
> > GCC will also splat the "indirect" value but not give you the register that 
> > the remaining structure is splatted into.  So ultimately, I think that 
> > filtering out any attempt to pass the PMF here is something that we 
> > can/should diagnose.  But should we do that for any aggregate type is 
> > questionable, and there is still the question of how you identify that the 
> > representation that the PMF will be lowered to is an aggregate.
> > But should we do that for any aggregate type is questionable, 
> 
> Yes, GCC seems to go out of its way to make a two-int aggregate work, but we 
> can't do that yet, and so far as I can tell, nobody is asking for that 
> ability, so I think it's OK to leave them alone for now.
> 
> >  there is still the question of how you identify that the representation 
> > that the PMF will be lowered to is an aggregate.
> 
> Right, I'm proposing we use the size of a pointer as a proxy for that. The 
> Microsoft ABI logic is quite complicated, and we wouldn't want to reimplement 
> it here. See the details:
> https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/MicrosoftCXXABI.cpp#L252
> 
> 
> Right, I'm proposing we use the size of a pointer as a proxy for that. The 
> Microsoft ABI logic is quite complicated, and we wouldn't want to reimplement 
> it here. See the details:

Am I reading that correctly in that member fields and not only member pointers 
can require the adjustment?  If not, we can tighten this up to 
`isMemberFunctionPointerType()`.  For now, this will simply not permit any PMF. 
 Happy to refine this further if you think that we should be a bit more 
generous here.


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

https://reviews.llvm.org/D138514

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

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 479752.
compnerd marked an inline comment as done.
compnerd added a comment.

Address feedback from review


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

https://reviews.llvm.org/D138514

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/Sema/gnu-asm-pmf.cpp


Index: clang/test/Sema/gnu-asm-pmf.cpp
===
--- /dev/null
+++ clang/test/Sema/gnu-asm-pmf.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++2b 
-fsyntax-only %s -verify
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -std=c++2b 
-fsyntax-only %s -verify
+
+struct S {
+  void operator()();
+};
+
+struct T {
+  virtual void operator()();
+};
+
+struct U {
+  static void operator()();
+};
+
+struct V: virtual T {
+  virtual void f();
+};
+
+struct W : virtual V {
+};
+
+struct X {
+  __UINTPTR_TYPE__ ptr;
+  __UINTPTR_TYPE__ adj;
+};
+
+auto L = [](){};
+
+void f() {
+  auto pmf = &S::operator();
+
+  __asm__ __volatile__ ("" : : "r"(&decltype(L)::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&S::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&T::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(pmf));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&W::f));
+  // expected-error@-1{{passing pointer-to-member through register constrained 
inline assembly parameter is not permitted}}
+
+  __asm__ __volatile__ ("" : : "r"(X{0,0}));
+  __asm__ __volatile__ ("" : : "r"(&U::operator()));
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -377,6 +377,11 @@
 
 Expr *InputExpr = Exprs[i];
 
+if (InputExpr->getType()->isMemberPointerType())
+  return StmtError(Diag(InputExpr->getBeginLoc(),
+diag::err_asm_pmf_through_constraint_not_permitted)
+   << InputExpr->getSourceRange());
+
 // Referring to parameters is not allowed in naked functions.
 if (CheckNakedParmReference(InputExpr, *this))
   return StmtError();
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8841,6 +8841,9 @@
 
 // inline asm.
 let CategoryName = "Inline Assembly Issue" in {
+  def err_asm_pmf_through_constraint_not_permitted
+: Error<"passing pointer-to-member through register constrained inline "
+"assembly parameter is not permitted">;
   def err_asm_invalid_lvalue_in_output : Error<"invalid lvalue in asm output">;
   def err_asm_invalid_output_constraint : Error<
 "invalid output constraint '%0' in asm">;


Index: clang/test/Sema/gnu-asm-pmf.cpp
===
--- /dev/null
+++ clang/test/Sema/gnu-asm-pmf.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++2b -fsyntax-only %s -verify
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -std=c++2b -fsyntax-only %s -verify
+
+struct S {
+  void operator()();
+};
+
+struct T {
+  virtual void operator()();
+};
+
+struct U {
+  static void operator()();
+};
+
+struct V: virtual T {
+  virtual void f();
+};
+
+struct W : virtual V {
+};
+
+struct X {
+  __UINTPTR_TYPE__ ptr;
+  __UINTPTR_TYPE__ adj;
+};
+
+auto L = [](){};
+
+void f() {
+  auto pmf = &S::operator();
+
+  __asm__ __volatile__ ("" : : "r"(&decltype(L)::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&S::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&T::operator()));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(pmf));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+  __asm__ __volatile__ ("" : : "r"(&W::f));
+  // expected-error@-1{{passing pointer-to-member through register constrained inline assembly parameter is not permitted}}
+
+  __asm

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

[Saleem Abdulrasool via Phabricator via cfe-commits]

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



Comment at: clang/test/Sema/gnu-asm-pmf.cpp:1-34
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++2b 
-fsyntax-only -verify %s -DMICROSOFT_ABI
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -std=c++2b 
-fsyntax-only -verify %s -DITANIUM_ABI
+
+#if defined(MICROSOFT_ABI)
+// expected-no-diagnostics
+#endif
+

aaron.ballman wrote:
> 
This is pretty amazing, I didn't know that `-verify` took an argument!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138514

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

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/lib/Sema/SemaStmtAsm.cpp:381
+if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
+  if (const auto *UO = dyn_cast(InputExpr))
+if (UO->getOpcode() == UO_AddrOf)

aaron.ballman wrote:
> rnk wrote:
> > This is too narrow, there are lots of other ways to do this:
> > ```
> > struct Foo { void method(); };
> > void f() {
> >   auto pmf = &Foo::method;
> >   asm volatile ("" : : "r"(pmf));
> > }
> > ```
> > 
> > I think it makes sense to check for:
> > * An expression with a member pointer type
> > * Where the size of the type is larger than the size of a pointer, or word, 
> > or whatever proxy we normally use for the size of a general purpose register
> > 
> > In the Microsoft ABI, member function pointers are only sometimes 
> > pointer-sized. If the class uses the multiple inheritance model, it will be 
> > bigger and include the this-adjuster field. See the inheritance keyword 
> > docs to learn more:
> > https://learn.microsoft.com/en-us/cpp/cpp/inheritance-keywords?view=msvc-170
> > 
> > This also handles large pointers to data members in the MS ABI, which also 
> > has a wacky aggregate representation.
> That sounds like a less fragile approach to me, thanks for that suggestion!
Thanks @rnk for pointing out the oversight on handling a PMF through a VarDecl 
and the pointer to the MSDN docs on how to form the adjusted reference.  This 
actually is more interesting.  Consider now the following:

```
struct s {
__UINTPTR_TYPE__ i, j;
};

void f() {
__asm__ __volatile__("" : : "r"(s{0,0}));
}

struct u { virtual void f(); };
struct v { virtual void operator()(); };
struct w: u, v {
};

void g() {
__asm__ __volatile__("" : : "r"(&w::operator()));
}
```

GCC does accept the input, but clang fails due to the backend failing to form 
the indirect passing for the aggregate (`Don't know how to handle indirect 
register inputs yet for constraint 'r'`).  Interestingly enough, when targeting 
`x86_64-unknown-windows-msvc` instead of `x86_64-unknown-linux-gnu`, we 
represent the PMF as an aggregate and trigger the same type of aggregate 
passing failure.  On Linux though we attempt to lower the aggregate passing and 
then fail to select the copy due to the aggregate being lowered into a single 
register rather than being passed as indirect.  One of the issues with the 
indirect passing is that GCC will also splat the "indirect" value but not give 
you the register that the remaining structure is splatted into.  So ultimately, 
I think that filtering out any attempt to pass the PMF here is something that 
we can/should diagnose.  But should we do that for any aggregate type is 
questionable, and there is still the question of how you identify that the 
representation that the PMF will be lowered to is an aggregate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138514

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

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added a reviewer: aaron.ballman.
compnerd added a project: clang.
Herald added a project: All.
compnerd requested review of this revision.

The itanium ABI represents the PMF as a pair of pointers.  As such the
structure cannot be passed through a single register.  Diagnose such
cases in the frontend rather than trying to generate IR to perform this
operation.

Fixes: 59033


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138514

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/Sema/gnu-asm-pmf.cpp


Index: clang/test/Sema/gnu-asm-pmf.cpp
===
--- /dev/null
+++ clang/test/Sema/gnu-asm-pmf.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++2b 
-fsyntax-only -verify %s -DMICROSOFT_ABI
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -std=c++2b 
-fsyntax-only -verify %s -DITANIUM_ABI
+
+#if defined(MICROSOFT_ABI)
+// expected-no-diagnostics
+#endif
+
+struct S {
+  void operator()() {}
+};
+
+struct T {
+  virtual void operator()() {}
+};
+
+struct U {
+  static void operator()() {}
+};
+
+auto L = [](){};
+
+void f() {
+  __asm__ __volatile__ ("" : : "r"(&decltype(L)::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on 
this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(&S::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on 
this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(&T::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on 
this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(U::operator()));
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -377,6 +377,16 @@
 
 Expr *InputExpr = Exprs[i];
 
+if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
+  if (const auto *UO = dyn_cast(InputExpr))
+if (UO->getOpcode() == UO_AddrOf)
+  if (const auto *DRE = dyn_cast(UO->getSubExpr()))
+if (const auto *CMD = dyn_cast(DRE->getDecl()))
+  if (CMD->isInstance())
+return StmtError(Diag(InputExpr->getBeginLoc(),
+  diag::err_asm_pmf_in_input)
+ << InputExpr->getSourceRange());
+
 // Referring to parameters is not allowed in naked functions.
 if (CheckNakedParmReference(InputExpr, *this))
   return StmtError();
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8842,6 +8842,8 @@
 
 // inline asm.
 let CategoryName = "Inline Assembly Issue" in {
+  def err_asm_pmf_in_input
+: Error<"cannot pass pointer-to-member through a register on this ABI">;
   def err_asm_invalid_lvalue_in_output : Error<"invalid lvalue in asm output">;
   def err_asm_invalid_output_constraint : Error<
 "invalid output constraint '%0' in asm">;


Index: clang/test/Sema/gnu-asm-pmf.cpp
===
--- /dev/null
+++ clang/test/Sema/gnu-asm-pmf.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++2b -fsyntax-only -verify %s -DMICROSOFT_ABI
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -std=c++2b -fsyntax-only -verify %s -DITANIUM_ABI
+
+#if defined(MICROSOFT_ABI)
+// expected-no-diagnostics
+#endif
+
+struct S {
+  void operator()() {}
+};
+
+struct T {
+  virtual void operator()() {}
+};
+
+struct U {
+  static void operator()() {}
+};
+
+auto L = [](){};
+
+void f() {
+  __asm__ __volatile__ ("" : : "r"(&decltype(L)::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(&S::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(&T::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(U::operator()));
+}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -377,6 +377,16 @@
 
 Expr *InputExpr = Exprs[i];
 
+if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
+  if (const auto *UO = dyn_cast(InputExpr))
+if (UO->getOpcode() == UO_AddrOf)
+  if (const auto *DRE = dyn

[PATCH] D138122: Lift EHPersonalities from Analysis to IR (NFC)

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd accepted this revision.
compnerd added a comment.

Additional validation is never a bad thing :).  Please do sort the headers 
before committing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138122

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

[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb78d5380da11: parse: process GNU and standard attributes on 
top-level decls (authored by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D137979?vs=476949&id=477006#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137979

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/Parser/attr-order.cpp
  clang/test/Parser/cxx-attributes.cpp
  clang/test/SemaCXX/attr-unavailable.cpp
  clang/unittests/Tooling/SourceCodeTest.cpp

Index: clang/unittests/Tooling/SourceCodeTest.cpp
===
--- clang/unittests/Tooling/SourceCodeTest.cpp
+++ clang/unittests/Tooling/SourceCodeTest.cpp
@@ -247,14 +247,25 @@
 
   // Includes attributes.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
 }
@@ -402,14 +413,25 @@
 
   // Includes attributes.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
 }
Index: clang/test/SemaCXX/attr-unavailable.cpp
===
--- clang/test/SemaCXX/attr-unavailable.cpp
+++ clang/test/SemaCXX/attr-unavailable.cpp
@@ -42,18 +42,18 @@
 // delayed process for 'deprecated'.
 //  and 
 enum DeprecatedEnum { DE_A, DE_B } __attribute__((deprecated)); // expected-note {{'DeprecatedEnum' has been explicitly marked deprecated here}}
-__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 typedef enum DeprecatedEnum AnotherDeprecatedEnum; // expected-warning {{'DeprecatedEnum' is deprecated}}
 
+__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 __attribute__((deprecated))
 DeprecatedEnum testDeprecated(DeprecatedEnum X) { return X; }
 
 
 enum UnavailableEnum { UE_A, UE_B } __attribute__((unavailable)); // expected-note {{'UnavailableEnum' has been explicitly marked unavailable here}}
-__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 typedef enum UnavailableEnum AnotherUnavailableEnum; // expected-error {{'UnavailableEnum' is unavailable}}
+ //
 
-
+__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 __attribute__((unavailable))
 UnavailableEnum testUnavailable(UnavailableEnum X) { return X; }
 
Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
+// GH#58229 - rejects-valid
+__attribute__((__visibility__("default"))) [[nodiscard]] int f();
+[[nodiscard]] __attribute__((__visibility__("default"))) int f();
+
 class c {
   virtual void f1(const char* a, ...)
 __attribute__ (( __format__(__printf__,2,3) )) = 0;
Index: clang/test/Parser/attr-order.cpp
===
--- clang/test/Parser/attr-order.cpp
+++ clang/test/Parser/attr-order.cpp
@@ -17,8 +17,8 @@
 __dec

[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 476949.
compnerd marked an inline comment as done.
compnerd added a comment.

Address review feedback


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

https://reviews.llvm.org/D137979

Files:
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/Parser/attr-order.cpp
  clang/test/Parser/cxx-attributes.cpp
  clang/test/SemaCXX/attr-unavailable.cpp
  clang/unittests/Tooling/SourceCodeTest.cpp

Index: clang/unittests/Tooling/SourceCodeTest.cpp
===
--- clang/unittests/Tooling/SourceCodeTest.cpp
+++ clang/unittests/Tooling/SourceCodeTest.cpp
@@ -247,14 +247,25 @@
 
   // Includes attributes.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
 }
@@ -402,14 +413,25 @@
 
   // Includes attributes.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
 }
Index: clang/test/SemaCXX/attr-unavailable.cpp
===
--- clang/test/SemaCXX/attr-unavailable.cpp
+++ clang/test/SemaCXX/attr-unavailable.cpp
@@ -42,18 +42,18 @@
 // delayed process for 'deprecated'.
 //  and 
 enum DeprecatedEnum { DE_A, DE_B } __attribute__((deprecated)); // expected-note {{'DeprecatedEnum' has been explicitly marked deprecated here}}
-__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 typedef enum DeprecatedEnum AnotherDeprecatedEnum; // expected-warning {{'DeprecatedEnum' is deprecated}}
 
+__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 __attribute__((deprecated))
 DeprecatedEnum testDeprecated(DeprecatedEnum X) { return X; }
 
 
 enum UnavailableEnum { UE_A, UE_B } __attribute__((unavailable)); // expected-note {{'UnavailableEnum' has been explicitly marked unavailable here}}
-__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 typedef enum UnavailableEnum AnotherUnavailableEnum; // expected-error {{'UnavailableEnum' is unavailable}}
+ //
 
-
+__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 __attribute__((unavailable))
 UnavailableEnum testUnavailable(UnavailableEnum X) { return X; }
 
Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
+// GH#58229 - rejects-valid
+__attribute__((__visibility__("default"))) [[nodiscard]] int f();
+[[nodiscard]] __attribute__((__visibility__("default"))) int f();
+
 class c {
   virtual void f1(const char* a, ...)
 __attribute__ (( __format__(__printf__,2,3) )) = 0;
Index: clang/test/Parser/attr-order.cpp
===
--- clang/test/Parser/attr-order.cpp
+++ clang/test/Parser/attr-order.cpp
@@ -17,8 +17,8 @@
 __declspec(dllexport) [[noreturn]] __attribute__((cdecl)) void d(); // expected-error {{an attribute list cannot appear here}}
 __declspec(dllexport) __attribute__((cdecl)) [[noreturn]] void e(); // expected-error {{an attribute list cannot appear here}}
 __attribute

[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd marked 5 inline comments as done.
compnerd added inline comments.



Comment at: clang/include/clang/Parse/Parser.h:1605-1606
   // C99 6.9: External Definitions.
   DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributes &Attrs,
+  ParsedAttributes &DeclSpecAttrs,
   ParsingDeclSpec *DS = nullptr);

aaron.ballman wrote:
> Should we rename `Attrs` to `DeclaratorAttrs` or something along those lines, 
> so it's easier for someone to distinguish which attributes go where? (Same 
> suggestion applies throughout the review.)
Yes, `DeclAttrs` would be much nicer, but I wanted to get everything right 
before doing that and then forgot.  Thanks for the reminder.



Comment at: clang/lib/Parse/Parser.cpp:1096-1097
+ParsingDeclSpec &DS, AccessSpecifier AS) {
+  DS.SetRangeStart(DeclSpecAttrs.Range.getBegin());
+  DS.SetRangeEnd(DeclSpecAttrs.Range.getEnd());
+  DS.takeAttributesFrom(DeclSpecAttrs);

aaron.ballman wrote:
> How sure are you that this isn't overwriting existing range data that's 
> potentially relevant? e.g., where declaration specifiers and attributes are 
> mixed together such that the attribute range doesn't cover all of the 
> declaration specifiers?
I don't believe it can - the range hasn't been setup yet when coming into this 
function, only the storage has been allocated.  This is the first place where 
we are initializing the source range.  If there are other unstated invariants, 
it would be nice to have some sort of assertion for them.


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

https://reviews.llvm.org/D137979

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

[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 476163.

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

https://reviews.llvm.org/D137979

Files:
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/Parser/attr-order.cpp
  clang/test/Parser/cxx-attributes.cpp
  clang/test/SemaCXX/attr-unavailable.cpp
  clang/unittests/Tooling/SourceCodeTest.cpp

Index: clang/unittests/Tooling/SourceCodeTest.cpp
===
--- clang/unittests/Tooling/SourceCodeTest.cpp
+++ clang/unittests/Tooling/SourceCodeTest.cpp
@@ -247,14 +247,25 @@
 
   // Includes attributes.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
 }
@@ -402,14 +413,25 @@
 
   // Includes attributes.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
 }
Index: clang/test/SemaCXX/attr-unavailable.cpp
===
--- clang/test/SemaCXX/attr-unavailable.cpp
+++ clang/test/SemaCXX/attr-unavailable.cpp
@@ -42,18 +42,18 @@
 // delayed process for 'deprecated'.
 //  and 
 enum DeprecatedEnum { DE_A, DE_B } __attribute__((deprecated)); // expected-note {{'DeprecatedEnum' has been explicitly marked deprecated here}}
-__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 typedef enum DeprecatedEnum AnotherDeprecatedEnum; // expected-warning {{'DeprecatedEnum' is deprecated}}
 
+__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 __attribute__((deprecated))
 DeprecatedEnum testDeprecated(DeprecatedEnum X) { return X; }
 
 
 enum UnavailableEnum { UE_A, UE_B } __attribute__((unavailable)); // expected-note {{'UnavailableEnum' has been explicitly marked unavailable here}}
-__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 typedef enum UnavailableEnum AnotherUnavailableEnum; // expected-error {{'UnavailableEnum' is unavailable}}
+ //
 
-
+__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 __attribute__((unavailable))
 UnavailableEnum testUnavailable(UnavailableEnum X) { return X; }
 
Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
+// GH#58229 - rejects-valid
+__attribute__((__visibility__("default"))) [[nodiscard]] int f();
+[[nodiscard]] __attribute__((__visibility__("default"))) int f();
+
 class c {
   virtual void f1(const char* a, ...)
 __attribute__ (( __format__(__printf__,2,3) )) = 0;
Index: clang/test/Parser/attr-order.cpp
===
--- clang/test/Parser/attr-order.cpp
+++ clang/test/Parser/attr-order.cpp
@@ -17,8 +17,8 @@
 __declspec(dllexport) [[noreturn]] __attribute__((cdecl)) void d(); // expected-error {{an attribute list cannot appear here}}
 __declspec(dllexport) __attribute__((cdecl)) [[noreturn]] void e(); // expected-error {{an attribute list cannot appear here}}
 __attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f(); // expected-error {{an attribute list 

[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 475570.
compnerd edited the summary of this revision.

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

https://reviews.llvm.org/D137979

Files:
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/Parser/attr-order.cpp
  clang/test/Parser/cxx-attributes.cpp
  clang/test/SemaCXX/attr-unavailable.cpp
  clang/test/SemaObjC/objc-asm-attribute-neg-test.m
  clang/unittests/Tooling/SourceCodeTest.cpp

Index: clang/unittests/Tooling/SourceCodeTest.cpp
===
--- clang/unittests/Tooling/SourceCodeTest.cpp
+++ clang/unittests/Tooling/SourceCodeTest.cpp
@@ -247,14 +247,25 @@
 
   // Includes attributes.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
 }
@@ -402,14 +413,25 @@
 
   // Includes attributes.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
 }
Index: clang/test/SemaObjC/objc-asm-attribute-neg-test.m
===
--- clang/test/SemaObjC/objc-asm-attribute-neg-test.m
+++ clang/test/SemaObjC/objc-asm-attribute-neg-test.m
@@ -28,7 +28,7 @@
 @end
 
 __attribute__((objc_runtime_name("MySecretNamespace.ForwardClass")))
-@class ForwardClass; // expected-error {{prefix attribute must be followed by an interface, protocol, or implementation}}
+@class ForwardClass; // expected-error@-1 {{prefix attribute must be followed by an interface, protocol, or implementation}}
 
 __attribute__((objc_runtime_name("MySecretNamespace.ForwardProtocol")))
 @protocol ForwardProtocol;
Index: clang/test/SemaCXX/attr-unavailable.cpp
===
--- clang/test/SemaCXX/attr-unavailable.cpp
+++ clang/test/SemaCXX/attr-unavailable.cpp
@@ -42,18 +42,18 @@
 // delayed process for 'deprecated'.
 //  and 
 enum DeprecatedEnum { DE_A, DE_B } __attribute__((deprecated)); // expected-note {{'DeprecatedEnum' has been explicitly marked deprecated here}}
-__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 typedef enum DeprecatedEnum AnotherDeprecatedEnum; // expected-warning {{'DeprecatedEnum' is deprecated}}
 
+__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 __attribute__((deprecated))
 DeprecatedEnum testDeprecated(DeprecatedEnum X) { return X; }
 
 
 enum UnavailableEnum { UE_A, UE_B } __attribute__((unavailable)); // expected-note {{'UnavailableEnum' has been explicitly marked unavailable here}}
-__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 typedef enum UnavailableEnum AnotherUnavailableEnum; // expected-error {{'UnavailableEnum' is unavailable}}
+ //
 
-
+__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 __attribute__((unavailable))
 UnavailableEnum testUnavailable(UnavailableEnum X) { return X; }
 
Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
+// GH#58229 - rejects-valid
+__attribute__((__visibilit

[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Add unsupported test cases


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

https://reviews.llvm.org/D137979

Files:
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant-varying-return.c
  clang/test/AST/ast-dump-openmp-begin-declare-variant_10.c
  clang/test/AST/ast-dump-openmp-begin-declare-variant_11.c
  clang/test/AST/ast-dump-openmp-begin-declare-variant_12.c
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objc.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objcpp.plist
  clang/test/Parser/attr-order.cpp
  clang/test/Parser/cxx-attributes.cpp
  clang/test/SemaCXX/attr-unavailable.cpp
  clang/test/SemaObjC/objc-asm-attribute-neg-test.m
  clang/unittests/Tooling/SourceCodeTest.cpp

Index: clang/unittests/Tooling/SourceCodeTest.cpp
===
--- clang/unittests/Tooling/SourceCodeTest.cpp
+++ clang/unittests/Tooling/SourceCodeTest.cpp
@@ -247,16 +247,29 @@
 
   // Includes attributes.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   // Comment.
   int x;]])cpp");
+
+#if TOOLING_MACRO_EXPANSION_UNSUPPORTED
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotate(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotate(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  // Comment.
+  int x;]])cpp");
+#endif
 }
 
 TEST(SourceCodeTest, getAssociatedRangeClasses) {
@@ -402,16 +415,29 @@
 
   // Includes attributes.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
+  // Comment.
+  int x;]])cpp");
+
+#if TOOLING_MACRO_EXPANSION_UNSUPPORTED
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotate(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
+  int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotate(R"cpp(
+  #define MACRO_EXPANSION __attribute__((deprecated("message")))
+  $r[[MACRO_EXPANSION
   // Comment.
   int x;]])cpp");
+#endif
 }
 
 TEST(SourceCodeTest, getAssociatedRangeInvalidForPartialExpansions) {
Index: clang/test/SemaObjC/objc-asm-attribute-neg-test.m
===
--- clang/test/SemaObjC/objc-asm-attribute-neg-test.m
+++ clang/test/SemaObjC/objc-asm-attribute-neg-test.m
@@ -28,7 +28,7 @@
 @end
 
 __attribute__((objc_runtime_name("MySecretNamespace.ForwardClass")))
-@class ForwardClass; // expected-error {{prefix attribute must be followed by an interface, protocol, or implementation}}
+@class ForwardClass; // expected-error@-1 {{prefix attribute must be followed by an interface, protocol, or implementation}}
 
 __attribute__((objc_runtime_name("MySecretNamespace.ForwardProtocol")))
 @protocol ForwardProtocol;
Index: clang/test/SemaCXX/attr-unavailable.cpp
===
--- clang/test/SemaCXX/attr-unavailable.cpp
+++ clang/test/SemaCXX/attr-unavailable.cpp
@@ -42,18 +42,18 @@
 // delayed process for 'deprecated'.
 //  and 
 enum DeprecatedEnum { DE_A, DE_B } __attribute__((deprecated)); // expected-note {{'DeprecatedEnum' has been explicitly marked deprecated here}}
-__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 typedef enum DeprecatedEnum AnotherDeprecatedEnum; // expected-warning {{'DeprecatedEnum' is deprecated}}
 
+__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 __attribute__((deprecated))
 DeprecatedEnum testDeprecated(DeprecatedEnum X) { return X; }
 
 
 enum UnavailableEnum { UE_A, UE_B } __attribute__((unavailable)); // expected-note {{'UnavailableEnum' has been explicitly marked unavailable here}}
-__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 typedef enum UnavailableEnu

[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:249-258
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visitor.runOverAnnotated(R"cpp(
+  $r[[__attribute__((deprecated("message")))

ymandel wrote:
> Thanks for trying to fix these! The changed test cases seem to be doing two 
> things at once: macros and attributes, and I don't remember why. We should 
> test the behavior separately.  So, I think you're new test cases are good 
> regardless.  But, we still want the old tests to test the behavior for 
> attributes that are hidden behind a macro, since this is used not 
> infrequently in my exprience. 
> 
> IIUC, the current code isn't properly handling attributes that appear inside 
> macros, which is why this fix breaks the code. Specifically, it is not 
> considering the case that the location in `Attr->getLocation()` is in a 
> macro, while `Range.getBegin()` is in the original file, and hence the 
> locations are not comparable. I'm surprised this ever worked, tbh.
> 
> My preference would be to update the code, if you know how, so that both the 
> old and new tests pass. But, I realize this may be a lot to ask. So, at the 
> least, please comment out, but keep, the old tests; or, copy the old tests to 
> a new test and mark it disabled. Either way, prefix with a FIXME indicating 
> the issue.
> 
> WDYT?
Unfortunately, I couldn't figure out how to track through the expansion so I am 
going to take you up on the commented out case option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137979

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

[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added reviewers: aaron.ballman, sammccall.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
compnerd requested review of this revision.
Herald added a project: clang.

We would previously reject valid input where GNU attributes preceded the 
standard attributes on top-level declarations.  A previous attribute handling 
change had begun rejecting this whilst GCC does honour this layout.  In 
practice, this breaks use of `extern "C"` attributed functions which use both 
standard and GNU attributes as experienced by the Swift runtime.

The majority of the changes here are plumbing the parsed attributes down to the 
use sites.  This incidentally improves source location tracking and token 
handling in the source manager.  The downside of this is that it exposes a 
latent issue in the indexing path where we would try to backtrack to annotate 
attributes post-expansion.  However the proper parsing now introduced results 
in the token stream now retrieving the pre-expanded token whilst still 
associating the attribute to the declaration.  This percolates throughout the 
other various tests.

The one remaining failure with this test is Index/annotate-tokens.c where the 
leading attribute is not associated with the declaration due to the 
`FinalizeDeclarationGroup` not re-associating the attributes with the 
declarations in the declaration group.  However, the declaration still does 
receive the attribute.

Special thanks to Aaron Ballman for the many hints and extensive rubber ducking 
that was involved in identifying the various places where we accidentally 
dropped attributes.

Fixes: #58229


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137979

Files:
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseHLSL.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant-varying-return.c
  clang/test/AST/ast-dump-openmp-begin-declare-variant_10.c
  clang/test/AST/ast-dump-openmp-begin-declare-variant_11.c
  clang/test/AST/ast-dump-openmp-begin-declare-variant_12.c
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objc.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objcpp.plist
  clang/test/Parser/attr-order.cpp
  clang/test/Parser/cxx-attributes.cpp
  clang/test/SemaCXX/attr-unavailable.cpp
  clang/test/SemaObjC/objc-asm-attribute-neg-test.m
  clang/unittests/Tooling/SourceCodeTest.cpp

Index: clang/unittests/Tooling/SourceCodeTest.cpp
===
--- clang/unittests/Tooling/SourceCodeTest.cpp
+++ clang/unittests/Tooling/SourceCodeTest.cpp
@@ -247,14 +247,12 @@
 
   // Includes attributes.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visitor.runOverAnnotated(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   // Comment.
   int x;]])cpp");
 }
@@ -402,14 +400,12 @@
 
   // Includes attributes.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   int x;]])cpp");
 
   // Includes attributes and comments together.
   Visit(R"cpp(
-  #define ATTR __attribute__((deprecated("message")))
-  $r[[ATTR
+  $r[[__attribute__((deprecated("message")))
   // Comment.
   int x;]])cpp");
 }
Index: clang/test/SemaObjC/objc-asm-attribute-neg-test.m
===
--- clang/test/SemaObjC/objc-asm-attribute-neg-test.m
+++ clang/test/SemaObjC/objc-asm-attribute-neg-test.m
@@ -28,7 +28,7 @@
 @end
 
 __attribute__((objc_runtime_name("MySecretNamespace.ForwardClass")))
-@class ForwardClass; // expected-error {{prefix attribute must be followed by an interface, protocol, or implementation}}
+@class ForwardClass; // expected-error@-1 {{prefix attribute must be followed by an interface, protocol, or implementation}}
 
 __attribute__((objc_runtime_name("MySecretNamespace.ForwardProtocol")))
 @protocol ForwardProtocol;
Index: clang/test/SemaCXX/attr-unavailable.cpp
===
--- clang/test/SemaCXX/attr-unavailable.cpp
+++ clang/test/SemaCXX/attr-unavailable.cpp
@@ -42,18 +42,18 @@
 // delayed process for 'deprecated'.
 //  and 
 enum DeprecatedEnum { DE_A, DE_B } __attribute__((deprecated)); // expected-note {{'DeprecatedEnum' has been explicitly marked deprecated here}}
-__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 typedef enum DeprecatedEnum AnotherDeprecatedEnum; // expected-warning {{'DeprecatedEnum' is deprecated}}

[PATCH] D133890: [CMake] Do these replacements to make use of D132608

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

I don't see anything wrong with this change per se, but I'm conflicted on the 
name of the variable.  These are not standard variables but are encroaching on 
the CMake namespace.  What happens if upstream decides to use these names?  I 
think that we should keep the names in the `LLVM_` namespace.  However, I 
realize that the variable itself is not being touched here and this is a 
mechanical replacement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133890

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

[PATCH] D132608: [CMake] Clean up CMake binary dir handling

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: cmake/Modules/GNUBinaryDirs.cmake:3
+  get_filename_component(CMAKE_LIBDIR_BASENAME "${CMAKE_INSTALL_LIBDIR}" NAME)
+endif()
+

Should this perhaps be moved further down near the usage?



Comment at: cmake/Modules/GNUBinaryDirs.cmake:6
+if (NOT DEFINED CMAKE_BINARY_BINDIR)
+  set(CMAKE_BINARY_BINDIR "${CMAKE_BINARY_BINDIR}/bin")
+endif()

arichardson wrote:
> I find this name a bit confusing, maybe something like `CMAKE_BUILD_BINDIR` 
> (and the same for _LIBDIR/_INCLUDEDIR would be less surprising? That way it's 
> clear that we are referring to binaries/libraries/includes in the build 
> output (and it mirrors CMAKE_INSTALL_INCLUDEDIR, etc.) 
I think you mean `${CMAKE_BINARY_DIR}/bin` as this is in a block where 
`CMAKE_BINARY_BINDIR` is undefined.



Comment at: cmake/Modules/GNUBinaryDirs.cmake:10
+if (NOT DEFINED CMAKE_BINARY_INCLUDEDIR)
+  set(CMAKE_BINARY_INCLUDEDIR "${CMAKE_BINARY_DIR}/inc")
+endif()

Is the default spelling for include not `include` not `inc`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132608

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

[PATCH] D130735: [Clang][LLD][cmake] Drop deprecated support for `llvm-config`-based build

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Since it has been deprecated for 2 releases, it seems reasonable to cleanup.  
This seems good to me from the build side, please do wait a bit for anyone else 
to chime in on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130735

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

[PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/include/clang/Config/config.h.cmake:57
+/* Multilib basename for libdir. */
+#define CLANG_INSTALL_LIBDIR_BASENAME "${CLANG_INSTALL_LIBDIR_BASENAME}"
 

Does this not potentially break downstreams?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[PATCH] D130446: [apinotes] Upstream changes to `APINotesYAMLCompiler.cpp`.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd requested changes to this revision.
compnerd added a comment.
This revision now requires changes to proceed.

Can you please add a round trip test as well?  Additionally, please add a 
proper commit message to describe the change here.




Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:425
+  Optional ReleaseOp;
+  FunctionsSeq MemberFuncs;
 };

Any reason to shorten `Functions` to `Funcs`?  It seems that it would be a bit 
odd given the other members.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130446

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

[PATCH] D130190: [Driver] Error for -gsplit-dwarf with RISC-V linker relaxation

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:146
+Arg *A;
+if (getDebugFissionKind(D, Args, A) != DwarfFissionKind::None)
+  D.Diag(clang::diag::err_drv_riscv_unsupported_with_linker_relaxation)

MaskRay wrote:
> compnerd wrote:
> > This should be hoisted out, this will occur with or without relaxation.  
> > That is, `-mrelax` and `-mno-relax` are both problematic.
> No. -mno-relax is supported with -gsplit-dwarf. I think the current code is 
> correct.
No, that is the point, it is not.  Building with `-mno-relax` and 
`-gsplit-dwarf` will fail with the same behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130190

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

[PATCH] D130190: [Driver] Error for -gsplit-dwarf with RISC-V linker relaxation

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:146
+Arg *A;
+if (getDebugFissionKind(D, Args, A) != DwarfFissionKind::None)
+  D.Diag(clang::diag::err_drv_riscv_unsupported_with_linker_relaxation)

This should be hoisted out, this will occur with or without relaxation.  That 
is, `-mrelax` and `-mno-relax` are both problematic.



Comment at: clang/test/Driver/riscv-features.c:41
+// RUN: not %clang -c --target=riscv64 -gsplit-dwarf=single %s 2>&1 | 
FileCheck %s --check-prefix=ERR-SPLIT-DWARF
+// RUN: %clang -### -c --target=riscv64 -mno-relax -g -gsplit-dwarf %s 2>&1 | 
FileCheck %s --check-prefix=SPLIT-DWARF
+

Can you please add a `-mno-relax` check as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130190

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

[PATCH] D130190: [Driver] Error for -gsplit-dwarf with RISC-V linker relaxation

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:10
 #include "RISCV.h"
+#include "../Clang.h"
 #include "ToolChains/CommonArgs.h"

This feels like a layering violation.  I suppose that as long as modular builds 
are okay ...



Comment at: clang/test/Driver/riscv-features.c:40
+// RUN: not %clang -c --target=riscv64-linux-gnu -gsplit-dwarf %s 2>&1 | 
FileCheck %s --check-prefix=ERR-SPLIT-DWARF
+// RUN: not %clang -c --target=riscv64 -gsplit-dwarf=single %s 2>&1 | 
FileCheck %s --check-prefix=ERR-SPLIT-DWARF
+// RUN: %clang -### -c --target=riscv64 -mno-relax -g -gsplit-dwarf %s 2>&1 | 
FileCheck %s --check-prefix=SPLIT-DWARF

Did you mean to add `-###` to these two?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130190

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

[PATCH] D126599: [docs][clang] Fix a broken link on the APINotes doc

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

It seems that other docs are following the GitHub URLs as well.  I wish we 
could do the relative document link instead of embedding the URL, but if that 
is not possible, then this makes sense to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126599

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

[PATCH] D126093: Sema: adjust assertion to account for deduced types

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
compnerd marked an inline comment as done.
Closed by commit rGb159108bc5eb: Sema: adjust assertion to account for deduced 
types (authored by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D126093?vs=431084&id=431720#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126093

Files:
  clang/include/clang/Sema/DeclSpec.h
  clang/test/Sema/typerep-typespec.c


Index: clang/test/Sema/typerep-typespec.c
===
--- /dev/null
+++ clang/test/Sema/typerep-typespec.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -std=c11 %s -fsyntax-only -verify
+// REQUIRES: asserts
+
+struct dispatch_object_s;
+void _dispatch_queue_get_head(struct dispatch_object_s *volatile 
dq_items_head) {
+  (_Atomic __typeof__(dq_items_head) *)0; // expected-warning{{expression 
result unused}}
+}
+void g(void) {
+  (_Atomic __typeof__(struct dispatch_object_s *volatile) *)0; // 
expected-warning{{expression result unused}}
+}
Index: clang/include/clang/Sema/DeclSpec.h
===
--- clang/include/clang/Sema/DeclSpec.h
+++ clang/include/clang/Sema/DeclSpec.h
@@ -516,7 +516,8 @@
   SourceLocation getTypeSpecSatLoc() const { return TSSatLoc; }
 
   SourceLocation getTypeSpecTypeNameLoc() const {
-assert(isDeclRep((TST) TypeSpecType) || TypeSpecType == TST_typename);
+assert(isDeclRep((TST)TypeSpecType) || isTypeRep((TST)TypeSpecType) ||
+   isExprRep((TST)TypeSpecType));
 return TSTNameLoc;
   }
 


Index: clang/test/Sema/typerep-typespec.c
===
--- /dev/null
+++ clang/test/Sema/typerep-typespec.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -std=c11 %s -fsyntax-only -verify
+// REQUIRES: asserts
+
+struct dispatch_object_s;
+void _dispatch_queue_get_head(struct dispatch_object_s *volatile dq_items_head) {
+  (_Atomic __typeof__(dq_items_head) *)0; // expected-warning{{expression result unused}}
+}
+void g(void) {
+  (_Atomic __typeof__(struct dispatch_object_s *volatile) *)0; // expected-warning{{expression result unused}}
+}
Index: clang/include/clang/Sema/DeclSpec.h
===
--- clang/include/clang/Sema/DeclSpec.h
+++ clang/include/clang/Sema/DeclSpec.h
@@ -516,7 +516,8 @@
   SourceLocation getTypeSpecSatLoc() const { return TSSatLoc; }
 
   SourceLocation getTypeSpecTypeNameLoc() const {
-assert(isDeclRep((TST) TypeSpecType) || TypeSpecType == TST_typename);
+assert(isDeclRep((TST)TypeSpecType) || isTypeRep((TST)TypeSpecType) ||
+   isExprRep((TST)TypeSpecType));
 return TSTNameLoc;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126093: Sema: adjust assertion to account for deduced types

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

git-clang-format


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

https://reviews.llvm.org/D126093

Files:
  clang/include/clang/Sema/DeclSpec.h
  clang/test/Sema/typerep-typespec.c


Index: clang/test/Sema/typerep-typespec.c
===
--- /dev/null
+++ clang/test/Sema/typerep-typespec.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -std=c11 -x c %s -fsyntax-only -verify
+// REQUIRES: asserts
+
+struct dispatch_object_s;
+void _dispatch_queue_get_head(struct dispatch_object_s *volatile 
dq_items_head) {
+  (_Atomic __typeof__(dq_items_head) *)0; // expected-warning{{expression 
result unused}}
+}
Index: clang/include/clang/Sema/DeclSpec.h
===
--- clang/include/clang/Sema/DeclSpec.h
+++ clang/include/clang/Sema/DeclSpec.h
@@ -516,7 +516,8 @@
   SourceLocation getTypeSpecSatLoc() const { return TSSatLoc; }
 
   SourceLocation getTypeSpecTypeNameLoc() const {
-assert(isDeclRep((TST) TypeSpecType) || TypeSpecType == TST_typename);
+assert(isDeclRep((TST)TypeSpecType) || isTypeRep((TST)TypeSpecType) ||
+   isExprRep((TST)TypeSpecType));
 return TSTNameLoc;
   }
 


Index: clang/test/Sema/typerep-typespec.c
===
--- /dev/null
+++ clang/test/Sema/typerep-typespec.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -std=c11 -x c %s -fsyntax-only -verify
+// REQUIRES: asserts
+
+struct dispatch_object_s;
+void _dispatch_queue_get_head(struct dispatch_object_s *volatile dq_items_head) {
+  (_Atomic __typeof__(dq_items_head) *)0; // expected-warning{{expression result unused}}
+}
Index: clang/include/clang/Sema/DeclSpec.h
===
--- clang/include/clang/Sema/DeclSpec.h
+++ clang/include/clang/Sema/DeclSpec.h
@@ -516,7 +516,8 @@
   SourceLocation getTypeSpecSatLoc() const { return TSSatLoc; }
 
   SourceLocation getTypeSpecTypeNameLoc() const {
-assert(isDeclRep((TST) TypeSpecType) || TypeSpecType == TST_typename);
+assert(isDeclRep((TST)TypeSpecType) || isTypeRep((TST)TypeSpecType) ||
+   isExprRep((TST)TypeSpecType));
 return TSTNameLoc;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126093: Sema: adjust assertion to account for deduced types

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added a reviewer: aaron.ballman.
compnerd added a project: clang.
Herald added a project: All.
compnerd requested review of this revision.

Previous changes for the BTF attributes introduced a new sub-tree
visitation.  That uncovered that when accessing the typespec location we
would assert that the type specification is either a type declaration or
`typename`.  However, `typename` was explicitly permitted.  This change
predates the introduction of newer deduced type representations such as
`__underlying_type` from C++ and the addition of the GNU `__typeof__`
expression.

Thanks to aaron.ballman for the valuable discussion and pointer to
`isTypeRep`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126093

Files:
  clang/include/clang/Sema/DeclSpec.h
  clang/test/Sema/typerep-typespec.c


Index: clang/test/Sema/typerep-typespec.c
===
--- /dev/null
+++ clang/test/Sema/typerep-typespec.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -std=c11 -x c %s -fsyntax-only -verify
+// REQUIRES: asserts
+
+struct dispatch_object_s;
+void _dispatch_queue_get_head(struct dispatch_object_s * volatile 
dq_items_head) {
+  (_Atomic __typeof__(dq_items_head) *)0; // expected-warning{{expression 
result unused}}
+}
Index: clang/include/clang/Sema/DeclSpec.h
===
--- clang/include/clang/Sema/DeclSpec.h
+++ clang/include/clang/Sema/DeclSpec.h
@@ -516,7 +516,8 @@
   SourceLocation getTypeSpecSatLoc() const { return TSSatLoc; }
 
   SourceLocation getTypeSpecTypeNameLoc() const {
-assert(isDeclRep((TST) TypeSpecType) || TypeSpecType == TST_typename);
+assert(isDeclRep((TST)TypeSpecType) || isTypeRep((TST)TypeSpecType) ||
+   isExprRep((TST)TypeSpecType));
 return TSTNameLoc;
   }
 


Index: clang/test/Sema/typerep-typespec.c
===
--- /dev/null
+++ clang/test/Sema/typerep-typespec.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -std=c11 -x c %s -fsyntax-only -verify
+// REQUIRES: asserts
+
+struct dispatch_object_s;
+void _dispatch_queue_get_head(struct dispatch_object_s * volatile dq_items_head) {
+  (_Atomic __typeof__(dq_items_head) *)0; // expected-warning{{expression result unused}}
+}
Index: clang/include/clang/Sema/DeclSpec.h
===
--- clang/include/clang/Sema/DeclSpec.h
+++ clang/include/clang/Sema/DeclSpec.h
@@ -516,7 +516,8 @@
   SourceLocation getTypeSpecSatLoc() const { return TSSatLoc; }
 
   SourceLocation getTypeSpecTypeNameLoc() const {
-assert(isDeclRep((TST) TypeSpecType) || TypeSpecType == TST_typename);
+assert(isDeclRep((TST)TypeSpecType) || isTypeRep((TST)TypeSpecType) ||
+   isExprRep((TST)TypeSpecType));
 return TSTNameLoc;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.
Herald added a project: All.

The sema portions of this change are still causing an issue.  Although the 
revert (rGf95bd18b5faa6a5af4b5786312c373c5b2dce687 
) and the 
subsequent re-application in rG3466e00716e12e32fdb100e3fcfca5c2b3e8d784 
  
addressed the debug info issue, the Sema issue remains.

  // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c11 -x c %s
  struct dispatch_object_s;
  void _dispatch_queue_get_head(struct dispatch_object_s * volatile 
dq_items_head) {
(_Atomic __typeof__(dq_items_head) *)dq_items_head;
  }

will trigger an assertion failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99

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

[PATCH] D121497: Lex: add support for `{,u}i128` Microsoft extension

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

I don't have an example module sadly.  It was something that I ran into with 
Swift code import the WinSDK module defined in 
https://github.com/apple/swift/blob/main/stdlib/public/Platform/winsdk.modulemap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121497

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

[PATCH] D121497: Lex: add support for `{,u}i128` Microsoft extension

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

This was something that I was hitting an issue with.  In particular, it was a 
module build for a module which pulled in intsafe.h.  Now, given that it is a 
preprocessor macro, it would stand to reason that it will normally be dropped 
and thus won't matter if the frontend doesn't actually support a 128-bit type.  
However, with a module, we would attempt to process the expression.  I do admit 
it is on a slightly more tenuous ground as Microsoft may somehow do something 
different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121497

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

[PATCH] D111579: [clang] Fix DIFile directory root on Windows

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/CodeGen/debug-prefix-map.c:7
+// RUN: %clang -g -fdebug-prefix-map=%p=%{fssrcroot}UNLIKELY_PATH%{fssep}empty 
-S -c %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang -g -ffile-prefix-map=%p=%{fssrcroot}UNLIKELY_PATH%{fssep}empty 
-S -c %s -emit-llvm -o - | FileCheck %s
 

I think that this illustrates my point on the base differential.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111579

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

[PATCH] D111457: [test] Add lit helper for windows paths

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Seems reasonable, though I'm not a fan of the variable names - they seem a bit 
difficult to read due to no separation (e.g., `%fs-src-root` or `%fs_src_root` 
vs `%fssrcroot`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111457

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

[PATCH] D121497: Lex: add support for `{,u}i128` Microsoft extension

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added a reviewer: aaron.ballman.
compnerd added a project: clang.
Herald added a project: All.
compnerd requested review of this revision.
Herald added a subscriber: cfe-commits.

The Windows SDK has occurrences of the `i128` and `ui128` suffix (at
least in the 10.0.22000.0 SDK, see `intsafe.h`).  This adds support for
the extension to properly lex the literal.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121497

Files:
  clang/include/clang/Lex/LiteralSupport.h
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/Lex/LiteralSupport.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Lexer/ms-extensions.c
  clang/unittests/AST/StmtPrinterTest.cpp

Index: clang/unittests/AST/StmtPrinterTest.cpp
===
--- clang/unittests/AST/StmtPrinterTest.cpp
+++ clang/unittests/AST/StmtPrinterTest.cpp
@@ -44,7 +44,7 @@
   PrintingPolicy Policy = Context->getPrintingPolicy();
   if (PolicyAdjuster)
 PolicyAdjuster(Policy);
-  S->printPretty(Out, /*Helper*/ nullptr, Policy);
+  S->printPretty(Out, /*Helper*/ nullptr, Policy, 0, "\n", Context);
 }
 
 template 
@@ -128,13 +128,15 @@
 "  1i8, -1i8, 1ui8, "
 "  1i16, -1i16, 1ui16, "
 "  1i32, -1i32, 1ui32, "
-"  1i64, -1i64, 1ui64;"
+"  1i64, -1i64, 1ui64, "
+"  1i128, -1i128, 1ui128;"
 "}",
 FunctionBodyMatcher("A"),
 "1i8 , -1i8 , 1Ui8 , "
 "1i16 , -1i16 , 1Ui16 , "
 "1 , -1 , 1U , "
-"1LL , -1LL , 1ULL"));
+"1LL , -1LL , 1ULL , "
+"1i128 , -1i128 , 1ui128"));
 // Should be: with semicolon
 }
 
Index: clang/test/Lexer/ms-extensions.c
===
--- clang/test/Lexer/ms-extensions.c
+++ clang/test/Lexer/ms-extensions.c
@@ -23,6 +23,9 @@
 #define USHORT 0xui16
 #define UCHAR 0xffui8
 
+#define INT128_MAX  170141183460469231731687303715884105727i128
+#define UINT128_MAX 0xui128
+
 void a(void) {
 	unsigned long long m = ULLONG_MAX;
 	unsigned int n = UINT;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -3924,6 +3924,11 @@
 
 // Get the value in the widest-possible width.
 unsigned MaxWidth = Context.getTargetInfo().getIntMaxTWidth();
+// Microsoft extensions allow a maximal integer greater than `intmax_t` via
+// the `i128` suffix, creating a 128-bit integeral value with a 64-bit
+// maximum integer type.
+if (Literal.MicrosoftInteger)
+  MaxWidth = Literal.MicrosoftInteger;
 llvm::APInt ResultVal(MaxWidth, 0);
 
 if (Literal.GetIntegerValue(ResultVal)) {
Index: clang/lib/Lex/LiteralSupport.cpp
===
--- clang/lib/Lex/LiteralSupport.cpp
+++ clang/lib/Lex/LiteralSupport.cpp
@@ -849,7 +849,7 @@
 case 'i':
 case 'I':
   if (LangOpts.MicrosoftExt && !isFPConstant) {
-// Allow i8, i16, i32, and i64. First, look ahead and check if
+// Allow i8, i16, i32, i64, and i128. First, look ahead and check if
 // suffixes are Microsoft integers and not the imaginary unit.
 uint8_t Bits = 0;
 size_t ToSkip = 0;
@@ -859,7 +859,10 @@
   ToSkip = 2;
   break;
 case '1':
-  if (s[2] == '6') { // i16 suffix
+  if (s[2] == '2' && s[3] == '8') {
+Bits = 128;
+ToSkip = 4;
+  } else if (s[2] == '6') { // i16 suffix
 Bits = 16;
 ToSkip = 3;
   }
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -1168,9 +1168,13 @@
   case BuiltinType::LongLong:  OS << "LL"; break;
   case BuiltinType::ULongLong: OS << "ULL"; break;
   case BuiltinType::Int128:
-break; // no suffix.
+if (Context && Context->getLangOpts().MicrosoftExt)
+  OS << "i128";
+break;
   case BuiltinType::UInt128:
-break; // no suffix.
+if (Context && Context->getLangOpts().MicrosoftExt)
+  OS << "ui128";
+break;
   }
 }
 
Index: clang/include/clang/Lex/LiteralSupport.h
===
--- clang/include/clang/Lex/LiteralSupport.h
+++ clang/include/clang/Lex/LiteralSupport.h
@@ -69,8 +69,8 @@
   bool isImaginary : 1; // 1.0i
   bool isFloat16 : 1;   // 1.0f16
   bool isFloat128 : 1;  // 1.0q
-  uint8_t MicrosoftInteger; // Microsoft suffix extension i8, i16, i32, or i64.
-
+  uint8_t MicrosoftInteger; // Microsoft suffix extension i8, i16, i32, i64, or
+// i128.
   bool isFract : 1; // 1.0hr/r/lr/uhr/ur/ulr
   bool isAccum : 1; // 1.0hk/k/lk/uhk/uk/ulk
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http

[PATCH] D116722: [clang] Verify ssp buffer size is a valid integer

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3204
   if (StackProtectorLevel) {
-CmdArgs.push_back("-stack-protector-buffer-size");
-// FIXME: Verify the argument is a valid integer.
-CmdArgs.push_back(Args.MakeArgString(Str.drop_front(16)));
+auto BufferSize = Str.drop_front(16);
+if (IsInteger(BufferSize)) {

I really am not a fan of the `16` here.  Why not just use `split` and split on 
`=`?  Or use [constexpr] `strlen`?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3209
+} else
+  D.Diag(clang::diag::err_invalid_ssp_buffer_size);
   }

Please consistently use the braces (either applied to both or on neither).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116722

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

[PATCH] D111457: [clang][test] Add lit helper for windows paths

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.



Comment at: clang/test/lit.cfg.py:60
+if platform.system() == 'Windows':
+root_sep = 'C:\\'
+else:

This isn't really a separator, this is the root itself.  I think that it makes 
sense to name it accordingly.  Please do not hard code this to `C:\`.  There is 
no guarantee that the root is `C:\`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111457

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

[PATCH] D113863: [clang-tidy] Make `readability-container-data-pointer` more robust

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:22
+constexpr llvm::StringLiteral DerefContainerExprName = "deref-container-expr";
+constexpr llvm::StringLiteral AddrofContainerExprName = 
"addrof-container-expr";
+constexpr llvm::StringLiteral AddressOfName = "address-of";

Would you mind using `addr-of-container-expr` and renaming this to 
`AddrOfContainerExprName`?



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:76
+arraySubscriptExpr(hasLHS(ContainerExpr), hasRHS(Zero))
+  .bind(AddressOfName),
   this);

Nice, I do like this, it is quite a bit more succinct.  One thing that is a bit 
less clear to me is that the previous expression was a bit more restrictive in 
the parameter types to certain expressions, is there a reason that you don't 
expect that to matter much in practice?  If a malformed input is provided, we 
could now match certain things that we didn't previously, or am I not matching 
up the conditions carefully enough?



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:99
+  Lexer::getSourceText(CharSourceRange::getTokenRange(SourceRange),
+   *Result.SourceManager, getLangOpts()));
+

The diff shows up odd here, is there a hard tab or is that just the rendering?


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

https://reviews.llvm.org/D113863

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

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/docs/UsersManual.rst:3546
 
-cmake -G"Visual Studio 15 2017" -T LLVM ..
+cmake -G"Visual Studio 17 2022" -T LLVM ..
 

Meinersbur wrote:
> jhenderson wrote:
> > RKSimon wrote:
> > > aaron.ballman wrote:
> > > > jhenderson wrote:
> > > > > I think the missing space should be fixed to :)
> > > > +1 to the missing space.
> > > This one is confusing - it isn't in my local diff, the raw diff, or if I 
> > > reapply the raw diff - it looks to have just appeared when you quote it 
> > > in phab comments?
> > The fact that the space is missing? It's missing in the current repo too.
> It works with and without the space, like `-GNinja` and `-G Ninja` both work.
It would be nice to mention the CMake minimum version.  I think that you need 
3.22 or newer for the 2022 generator/toolset definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114639

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

[PATCH] D112890: headers: optionalise some generated resource headers

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

@craig.topper - ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112890

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

[PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.



Comment at: llvm/CMakeLists.txt:289
+set(LLVM_TOOLS_INSTALL_DIR "${CMAKE_INSTALL_BINDIR}" CACHE STRING
+"Path for binary subdirectory (defaults to 'bin')")
 mark_as_advanced(LLVM_TOOLS_INSTALL_DIR)





Comment at: llvm/docs/CMake.rst:270
+  Defaults to ``share/man``.
+
 .. _LLVM-related variables:

I'm kinda torn on this.  The variables here are all CMake standard and we 
should redirect users to CMake.  Although, this is following the existing 
pattern, so seems reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810

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

[PATCH] D112890: headers: optionalise some generated resource headers

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

In D112890#3099256 , @tschuett wrote:

> If I understand you correctly, I would need to pass something ala `-target 
> riscv-xx` to  enable `__riscv_vector`. However, this is impossible because 
> the risk target is disabled. So I thing it is save what you are doing.

Correct.  You would need to invoke clang as `clang -target 
riscv64-unknown-linux-musl -march=rv64gv0p10 -menable-experimental-extensions` 
as a concrete example.  At that point, you would be able to include 
`riscv_vector.h` for the declarations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112890

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

[PATCH] D112890: headers: optionalise some generated resource headers

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

In D112890#3099208 , @tschuett wrote:

> I am undecided between this may break something and I cannot use the risc-v 
> headers on an x86 machine.

This fundamentally cannot break something outside of the RISC-V //target//, it 
is applicable to any and all hosts.  However, the condition ensures that RISC-V 
support is not enabled when building the compiler.

  #ifndef __riscv_vector
  #error "Vector intrinsics require the vector extension."
  #endif

`__riscv_vector` is in the implementation's namespace, so the user may not 
define this (it is defined by the compiler when targeting a RISCV core with the 
V extension).  Trying to use `-target riscv32-unknown-*-*` or `-target 
riscv64-unknown-*-*` (not literally, but with a concrete value), will fail due 
to RISCV support being disabled in the build.  That is, the header cannot be 
used on any machine when not targeting RISCV.  Because RISCV is not enabled, it 
is safe to elide the header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112890

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

[PATCH] D112890: headers: optionalise some generated resource headers

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added a reviewer: craig.topper.
Herald added subscribers: luke957, luismarques, s.egerton, PkmX, simoncook, 
kristof.beyls, mgorny.
compnerd requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This splits out the generated headers and conditonalises them upon the
target being enabled.

The motivation here is that the RISCV header alone added 10MB to the
resource directory, which was previously at 10MB, increasing the build
size and time.  This header is contributing ~50% of the size of the
resource headers (~10MB).

The ARM generated headers are contributing about ~10% or 1MB.

This could be extended further adding only the static resource headers
for the targets that the LLVM build supports.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112890

Files:
  clang/lib/Headers/CMakeLists.txt


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -206,20 +206,24 @@
 endforeach( f )
 
 # Generate header files and copy them to the build directory
-# Generate arm_neon.h
-clang_generate_header(-gen-arm-neon arm_neon.td arm_neon.h)
-# Generate arm_fp16.h
-clang_generate_header(-gen-arm-fp16 arm_fp16.td arm_fp16.h)
-# Generate arm_sve.h
-clang_generate_header(-gen-arm-sve-header arm_sve.td arm_sve.h)
-# Generate arm_bf16.h
-clang_generate_header(-gen-arm-bf16 arm_bf16.td arm_bf16.h)
-# Generate arm_mve.h
-clang_generate_header(-gen-arm-mve-header arm_mve.td arm_mve.h)
-# Generate arm_cde.h
-clang_generate_header(-gen-arm-cde-header arm_cde.td arm_cde.h)
-# Generate riscv_vector.h
-clang_generate_header(-gen-riscv-vector-header riscv_vector.td riscv_vector.h)
+if(ARM IN_LIST LLVM_TARGETS_TO_BUILD OR AArch64 IN_LIST LLVM_TARGETS_TO_BUILD)
+  # Generate arm_neon.h
+  clang_generate_header(-gen-arm-neon arm_neon.td arm_neon.h)
+  # Generate arm_fp16.h
+  clang_generate_header(-gen-arm-fp16 arm_fp16.td arm_fp16.h)
+  # Generate arm_sve.h
+  clang_generate_header(-gen-arm-sve-header arm_sve.td arm_sve.h)
+  # Generate arm_bf16.h
+  clang_generate_header(-gen-arm-bf16 arm_bf16.td arm_bf16.h)
+  # Generate arm_mve.h
+  clang_generate_header(-gen-arm-mve-header arm_mve.td arm_mve.h)
+  # Generate arm_cde.h
+  clang_generate_header(-gen-arm-cde-header arm_cde.td arm_cde.h)
+endif()
+if(RISCV IN_LIST LLVM_TARGETS_TO_BUILD)
+  # Generate riscv_vector.h
+  clang_generate_header(-gen-riscv-vector-header riscv_vector.td 
riscv_vector.h)
+endif()
 
 add_custom_target(clang-resource-headers ALL DEPENDS ${out_files})
 set_target_properties(clang-resource-headers PROPERTIES


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -206,20 +206,24 @@
 endforeach( f )
 
 # Generate header files and copy them to the build directory
-# Generate arm_neon.h
-clang_generate_header(-gen-arm-neon arm_neon.td arm_neon.h)
-# Generate arm_fp16.h
-clang_generate_header(-gen-arm-fp16 arm_fp16.td arm_fp16.h)
-# Generate arm_sve.h
-clang_generate_header(-gen-arm-sve-header arm_sve.td arm_sve.h)
-# Generate arm_bf16.h
-clang_generate_header(-gen-arm-bf16 arm_bf16.td arm_bf16.h)
-# Generate arm_mve.h
-clang_generate_header(-gen-arm-mve-header arm_mve.td arm_mve.h)
-# Generate arm_cde.h
-clang_generate_header(-gen-arm-cde-header arm_cde.td arm_cde.h)
-# Generate riscv_vector.h
-clang_generate_header(-gen-riscv-vector-header riscv_vector.td riscv_vector.h)
+if(ARM IN_LIST LLVM_TARGETS_TO_BUILD OR AArch64 IN_LIST LLVM_TARGETS_TO_BUILD)
+  # Generate arm_neon.h
+  clang_generate_header(-gen-arm-neon arm_neon.td arm_neon.h)
+  # Generate arm_fp16.h
+  clang_generate_header(-gen-arm-fp16 arm_fp16.td arm_fp16.h)
+  # Generate arm_sve.h
+  clang_generate_header(-gen-arm-sve-header arm_sve.td arm_sve.h)
+  # Generate arm_bf16.h
+  clang_generate_header(-gen-arm-bf16 arm_bf16.td arm_bf16.h)
+  # Generate arm_mve.h
+  clang_generate_header(-gen-arm-mve-header arm_mve.td arm_mve.h)
+  # Generate arm_cde.h
+  clang_generate_header(-gen-arm-cde-header arm_cde.td arm_cde.h)
+endif()
+if(RISCV IN_LIST LLVM_TARGETS_TO_BUILD)
+  # Generate riscv_vector.h
+  clang_generate_header(-gen-riscv-vector-header riscv_vector.td riscv_vector.h)
+endif()
 
 add_custom_target(clang-resource-headers ALL DEPENDS ${out_files})
 set_target_properties(clang-resource-headers PROPERTIES
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

@thakis - thanks, seems that I had a part of the change sitting in my stash ... 
I had added a `-NOT` to verify the behaviour, and forgot to remove it.  I'll 
revert the revert with the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

@Eugene.Zelenko - sorry, I didn't see the additional comments before the 
commit.  I'm happy to do a follow up depending on the resolution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
compnerd marked an inline comment as done.
Closed by commit rGd0d9e6f0849b: clang-tidy: introduce 
readability-containter-data-pointer check (authored by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D108893?vs=372285&id=372485#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template 
+struct vector {
+  using size_type = size_t;
+
+  vector();
+  explicit vector(size_type);
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+};
+
+template 
+struct basic_string {
+  using size_type = size_t;
+
+  basic_string();
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_t);
+  const T &operator[](size_type) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+
+template 
+struct is_integral;
+
+template <>
+struct is_integral {
+  static const bool value = true;
+};
+
+template 
+struct enable_if { };
+
+template 
+struct enable_if {
+  typedef T type;
+};
+}
+
+template 
+void f(const T *);
+
+#define z (0)
+
+void g(size_t s) {
+  std::vector b(s);
+  f(&((b)[(z)]));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+void h() {
+  std::string s;
+  f(&((s).operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(s.data());{{$}}
+
+  std::wstring w;
+  f(&((&(w))->operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(w.data());{{$}}
+}
+
+template ::value>::type>
+void i(U s) {
+  std::vector b(s);
+  f(&b[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+template void i(size_t);
+
+void j(std::vector * const v) {
+  f(&(*v)[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v->data());{{$}}
+}
+
+void k(const std::vector &v) {
+  f(&v[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v.data());{{$}}
+}
+
+void l() {
+  unsigned char b[32];
+  f(&b[0]);
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+}
+
+template 
+void m(const std::vector &v) {
+  const T *p = &v[0];
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:16: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - readability-data-pointer
+
+readability-data-pointer
+
+
+Finds cases where code could use ``data()`` rather than the address of the
+element at index 0 in a container.  This pattern is commonly used to materialize
+a pointer to the backing data of a container.  ``std::vector`` and
+``std::string`` provide a ``data()`` accessor 

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp:104
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'data' should be used for 
accessing the data pointer instead of taking the address of the 0-th element 
[readability-container-data-pointer]
+}

aaron.ballman wrote:
> Can you add one more test for how this behaves on uninstantiated templates?
> ```
> template 
> void func(const std::vector &Vec) {
>   const Ty *Ptr = &Vec[0]; // Should diagnose and provide a fix-it despite 
> not instantiating func()
> }
> ```
Absolutely!  Thanks for the additional test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Address review feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template 
+struct vector {
+  using size_type = size_t;
+
+  vector();
+  explicit vector(size_type);
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+};
+
+template 
+struct basic_string {
+  using size_type = size_t;
+
+  basic_string();
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_t);
+  const T &operator[](size_type) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+
+template 
+struct is_integral;
+
+template <>
+struct is_integral {
+  static const bool value = true;
+};
+
+template 
+struct enable_if { };
+
+template 
+struct enable_if {
+  typedef T type;
+};
+}
+
+template 
+void f(const T *);
+
+#define z (0)
+
+void g(size_t s) {
+  std::vector b(s);
+  f(&((b)[(z)]));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+void h() {
+  std::string s;
+  f(&((s).operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(s.data());{{$}}
+
+  std::wstring w;
+  f(&((&(w))->operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(w.data());{{$}}
+}
+
+template ::value>::type>
+void i(U s) {
+  std::vector b(s);
+  f(&b[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+template void i(size_t);
+
+void j(std::vector * const v) {
+  f(&(*v)[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v->data());{{$}}
+}
+
+void k(const std::vector &v) {
+  f(&v[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v.data());{{$}}
+}
+
+void l() {
+  unsigned char b[32];
+  f(&b[0]);
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - readability-data-pointer
+
+readability-data-pointer
+
+
+Finds cases where code could use ``data()`` rather than the address of the
+element at index 0 in a container.  This pattern is commonly used to materialize
+a pointer to the backing data of a container.  ``std::vector`` and
+``std::string`` provide a ``data()`` accessor to retrieve the data pointer which
+should be preferred.
+
+This also ensures that in the case that the container is empty, the data pointer
+access does not perform an errant memory access.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -91,6 +91,11 @@
   variables and function parameters only.
 
 
+- New :doc:`readability-dat

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd marked 7 inline comments as done.
compnerd added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:63
+hasType(Container),
+hasType(pointsTo(Container)),
+hasType(references(Container))

aaron.ballman wrote:
> Same question here about `pointsTo()`.
This is actually meant to catch cases such as references to UnOp(DRE).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

@Eugene.Zelenko I'd like to have @aaron.ballman weigh in on the naming, and 
assuming that he's okay with `modernize.container-data-pointer`, I'll change it 
to that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 369453.
compnerd marked 2 inline comments as done.
compnerd added a comment.

Update release notes to incorporate feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template 
+struct vector {
+  using size_type = size_t;
+
+  vector();
+  explicit vector(size_type);
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+};
+
+template 
+struct basic_string {
+  using size_type = size_t;
+
+  basic_string();
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_t);
+  const T &operator[](size_type) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+
+template 
+struct is_integral;
+
+template <>
+struct is_integral {
+  static const bool value = true;
+};
+
+template 
+struct enable_if { };
+
+template 
+struct enable_if {
+  typedef T type;
+};
+}
+
+template 
+void f(const T *);
+
+#define z (0)
+
+void g(size_t s) {
+  std::vector b(s);
+  f(&((b)[(z)]));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+void h() {
+  std::string s;
+  f(&((s).operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(s.data());{{$}}
+
+  std::wstring w;
+  f(&((&(w))->operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(w.data());{{$}}
+}
+
+template ::value>::type>
+void i(U s) {
+  std::vector b(s);
+  f(&b[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+template void i(size_t);
+
+void j(std::vector * const v) {
+  f(&(*v)[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v->data());{{$}}
+}
+
+void k(const std::vector &v) {
+  f(&v[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v.data());{{$}}
+}
+
+void l() {
+  unsigned char b[32];
+  f(&b[0]);
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - readability-data-pointer
+
+readability-data-pointer
+
+
+Finds cases where code could use ``data()`` rather than the address of the
+element at index 0 in a container.  This pattern is commonly used to materialize
+a pointer to the backing data of a container.  ``std::vector`` and
+``std::string`` provide a ``data()`` accessor to retrieve the data pointer which
+should be preferred.
+
+This also ensures that in the case that the container is empty, the data pointer
+access does not perform an errant memory access.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -82,6 +82,11 @@
 
   Repor

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd marked 2 inline comments as done.
compnerd added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87
+
+  Finds cases where code could use ``data`` rather than the address of an 
element.
+

Eugene.Zelenko wrote:
> It'll be good idea to mention containers and that `data` is method.
That was dumb, you're right.  Thanks!



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst:6
+
+Finds instances of the data pointer being materialized by taking the address of
+the element at index 0 of a container. In particular, ``std::vector`` and

Eugene.Zelenko wrote:
> Please make first statement same as in Release Notes.
That was a good idea.  I find that the updated text reads much better too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 369343.
compnerd edited the summary of this revision.
compnerd added a comment.

Add some documentation and release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template 
+struct vector {
+  using size_type = size_t;
+
+  vector();
+  explicit vector(size_type);
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+};
+
+template 
+struct basic_string {
+  using size_type = size_t;
+
+  basic_string();
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_t);
+  const T &operator[](size_type) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+
+template 
+struct is_integral;
+
+template <>
+struct is_integral {
+  static const bool value = true;
+};
+
+template 
+struct enable_if { };
+
+template 
+struct enable_if {
+  typedef T type;
+};
+}
+
+template 
+void f(const T *);
+
+#define z (0)
+
+void g(size_t s) {
+  std::vector b(s);
+  f(&((b)[(z)]));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+void h() {
+  std::string s;
+  f(&((s).operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(s.data());{{$}}
+
+  std::wstring w;
+  f(&((&(w))->operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(w.data());{{$}}
+}
+
+template ::value>::type>
+void i(U s) {
+  std::vector b(s);
+  f(&b[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+template void i(size_t);
+
+void j(std::vector * const v) {
+  f(&(*v)[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v->data());{{$}}
+}
+
+void k(const std::vector &v) {
+  f(&v[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v.data());{{$}}
+}
+
+void l() {
+  unsigned char b[32];
+  f(&b[0]);
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - readability-data-pointer
+
+readability-data-pointer
+
+
+Finds instances of the data pointer being materialized by taking the address of
+the element at index 0 of a container. In particular, ``std::vector`` and
+``std::string`` provide a ``data()`` accessor to retrieve the data pointer which
+should be preferred.
+
+This also ensures that in the case that the container is empty, the data pointer
+access does not perform an errant memory access.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -82,6 +82,10 @@
 
   Reports identifiers whose names are too short. Currently checks local variab

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Hmm, one case that doesn't currently get handled properly is the following test 
case:

  c++
  template  void f(const T *);
  void g(const std::vector **v) {
f(&(**v)[0]);
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:18
+namespace tidy {
+namespace readability {
+ContainerDataPointerCheck::ContainerDataPointerCheck(StringRef Name,

Eugene.Zelenko wrote:
> compnerd wrote:
> > Eugene.Zelenko wrote:
> > > Please separate with empty line.
> > Hmm, this is what clang-format does, which should be unambiguously correct. 
> >  Is there something that needs to be changed in clang-format or 
> > .clang-format?
> Hard to tell about current state of clang-format, but you could take a look 
> on existing checks code.
I would prefer that we fix the clang-format configuration rather than manually 
change the formatting here.  This is introducing a new set of files, so it 
isn't resulting in inconsistency in a single file.  I don't mind reformatting 
the code with clang-format, but generally, that is considered the definitive 
arbiter for formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

In D108893#2971410 , @Eugene.Zelenko 
wrote:

> Thank you for implementing 26817 
> ! But shouldn't this check 
> belong to `modernize` module?

Oh, I was unaware of the PR, I'll tag that in the commit message, thanks!

Hmm, I'm somewhat on the fence, but I wouldn't be against the reorganization to 
`modernize`, only I would prefer that we get that settled before I do the 
actual rename/move.

> Please add documentation and mention new check in Release Notes.

Ah, good idea, I'll add that as well.




Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:18
+namespace tidy {
+namespace readability {
+ContainerDataPointerCheck::ContainerDataPointerCheck(StringRef Name,

Eugene.Zelenko wrote:
> Please separate with empty line.
Hmm, this is what clang-format does, which should be unambiguously correct.  Is 
there something that needs to be changed in clang-format or .clang-format?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 369340.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template 
+struct vector {
+  using size_type = size_t;
+
+  vector();
+  explicit vector(size_type);
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+};
+
+template 
+struct basic_string {
+  using size_type = size_t;
+
+  basic_string();
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_t);
+  const T &operator[](size_type) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+
+template 
+struct is_integral;
+
+template <>
+struct is_integral {
+  static const bool value = true;
+};
+
+template 
+struct enable_if { };
+
+template 
+struct enable_if {
+  typedef T type;
+};
+}
+
+template 
+void f(const T *);
+
+#define z (0)
+
+void g(size_t s) {
+  std::vector b(s);
+  f(&((b)[(z)]));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+void h() {
+  std::string s;
+  f(&((s).operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(s.data());{{$}}
+
+  std::wstring w;
+  f(&((&(w))->operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(w.data());{{$}}
+}
+
+template ::value>::type>
+void i(U s) {
+  std::vector b(s);
+  f(&b[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+template void i(size_t);
+
+void j(std::vector * const v) {
+  f(&(*v)[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v->data());{{$}}
+}
+
+void k(const std::vector &v) {
+  f(&v[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v.data());{{$}}
+}
+
+void l() {
+  unsigned char b[32];
+  f(&b[0]);
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+}
Index: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -12,6 +12,7 @@
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
+#include "ContainerDataPointerCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ConvertMemberFunctionsToStatic.h"
 #include "DeleteNullPointerCheck.h"
@@ -62,6 +63,8 @@
 "readability-braces-around-statements");
 CheckFactories.registerCheck(
 "readability-const-return-type");
+CheckFactories.registerCheck(
+"readability-container-data-pointer");
 CheckFactories.registerCheck(
 "readability-container-size-empty");
 CheckFactories.registerCheck(
Index: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
@@ -0,0 +1,45 @@
+//===--- ContainerDataPointerCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Projec

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Reflow the text using clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t -- -- -fno-delayed-template-parsing
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template 
+struct vector {
+  using size_type = size_t;
+
+  vector();
+  explicit vector(size_type);
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+};
+
+template 
+struct basic_string {
+  using size_type = size_t;
+
+  basic_string();
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_t);
+  const T &operator[](size_type) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+
+template 
+struct is_integral;
+
+template <>
+struct is_integral {
+  static const bool value = true;
+};
+
+template 
+struct enable_if { };
+
+template 
+struct enable_if {
+  typedef T type;
+};
+}
+
+template 
+void f(const T *);
+
+#define z (0)
+
+void g(size_t s) {
+  std::vector b(s);
+  f(&((b)[(z)]));
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+void h() {
+  std::string s;
+  f(&((s).operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(s.data());{{$}}
+
+  std::wstring w;
+  f(&((&(w))->operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(w.data());{{$}}
+}
+
+template ::value>::type>
+void i(U s) {
+  std::vector b(s);
+  f(&b[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+template void i(size_t);
+
+void j(std::vector * const v) {
+  f(&(*v)[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v->data());{{$}}
+}
+
+void k(const std::vector &v) {
+  f(&v[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v.data());{{$}}
+}
+
+void l() {
+  unsigned char b[32];
+  f(&b[0]);
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+}
Index: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -12,6 +12,7 @@
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
+#include "ContainerDataPointerCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ConvertMemberFunctionsToStatic.h"
 #include "DeleteNullPointerCheck.h"
@@ -62,6 +63,8 @@
 "readability-braces-around-statements");
 CheckFactories.registerCheck(
 "readability-const-return-type");
+CheckFactories.registerCheck(
+"readability-container-data-pointer");
 CheckFactories.registerCheck(
 "readability-container-size-empty");
 CheckFactories.registerCheck(
Index: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
@@ -0,0 +1,45 @@
+//===--- ContainerDataPoin

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added a reviewer: aaron.ballman.
Herald added a subscriber: mgorny.
compnerd requested review of this revision.
Herald added a project: clang-tools-extra.

This introduces a new check, readability-containter-data-pointer.  This
check is meant to catch the cases where the user may be trying to
materialize the data pointer by taking the address of the 0-th member of
a container.  With C++11 or newer, the `data` member should be used for
this.  This provides the following benefits:

- `.data()` is easier to read than `&[0]`
- it avoids an unnecessary re-materialization of the pointer
  - this doesn't matter in the case of optimized code, but in the case of 
unoptimized code, this will be visible
- it avoids a potential invalid memory de-reference caused by the indexing when 
the container is empty (in debug mode, clang will normally optimize away the 
re-materialization in optimized builds).

The small potential behavioural change raises the question of where the
check should belong.  A reasoning of defense in depth applies here, and
this does an unchecked conversion, with the assumption that users can
use the static analyzer to catch cases where we can statically identify
an invalid memory de-reference.  For the cases where the static analysis
is unable to prove the size of the container, UBSan can be used to track
the invalid access.

Special thanks to Aaron Ballman for the discussion on whether this
check would be useful and where to place it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108893

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t -- -- -fno-delayed-template-parsing
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template 
+struct vector {
+  using size_type = size_t;
+
+  vector();
+  explicit vector(size_type);
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+};
+
+template 
+struct basic_string {
+  using size_type = size_t;
+
+  basic_string();
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_t);
+  const T &operator[](size_type) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+
+template 
+struct is_integral;
+
+template <>
+struct is_integral {
+  static const bool value = true;
+};
+
+template 
+struct enable_if { };
+
+template 
+struct enable_if {
+  typedef T type;
+};
+}
+
+template 
+void f(const T *);
+
+#define z (0)
+
+void g(size_t s) {
+  std::vector b(s);
+  f(&((b)[(z)]));
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+void h() {
+  std::string s;
+  f(&((s).operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(s.data());{{$}}
+
+  std::wstring w;
+  f(&((&(w))->operator[]((z;
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(w.data());{{$}}
+}
+
+template ::value>::type>
+void i(U s) {
+  std::vector b(s);
+  f(&b[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(b.data());{{$}}
+}
+
+template void i(size_t);
+
+void j(std::vector * const v) {
+  f(&(*v)[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v->data());{{$}}
+}
+
+void k(const std::vector &v) {
+  f(&v[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+  // CHECK-FIXES: {{^  }}f(v.data());{{$}}
+}
+
+void l() {
+  unsigned char b[32];
+  f(&b[0]);
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]: warning: 'data' s

[PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: llvm/CMakeLists.txt:589
 CACHE STRING "Doxygen-generated HTML documentation install directory")
-set(LLVM_INSTALL_OCAMLDOC_HTML_DIR "share/doc/llvm/ocaml-html"
+set(LLVM_INSTALL_OCAMLDOC_HTML_DIR 
"${CMAKE_INSTALL_DOCDIR}/${project}/ocaml-html"
 CACHE STRING "OCamldoc-generated HTML documentation install directory")

Why the change from `llvm` -> `${project}`?  (not that it really makes a 
difference)



Comment at: llvm/cmake/modules/AddSphinxTarget.cmake:77
 if (CMAKE_INSTALL_MANDIR)
-  set(INSTALL_MANDIR ${CMAKE_INSTALL_MANDIR}/)
+  set(INSTALL_MANDIR "${CMAKE_INSTALL_MANDIR}/")
 else()

Nit: trailing slash is unnecessary, `CMAKE_INSTALL_MANDIR` should be defined, 
and if not, you do not want installation into `/` anyway.



Comment at: llvm/cmake/modules/CMakeLists.txt:3
 
-set(LLVM_INSTALL_PACKAGE_DIR lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm)
+set(LLVM_INSTALL_PACKAGE_DIR lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm CACHE STRING 
"Path for CMake subdirectory (defaults to 'cmake/llvm')")
 set(llvm_cmake_builddir "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}")

Why is this variable being put into the cache now?



Comment at: llvm/cmake/modules/LLVMInstallSymlink.cmake:7
   set(DESTDIR $ENV{DESTDIR})
-  set(bindir "${DESTDIR}${CMAKE_INSTALL_PREFIX}/${outdir}/")
+  set(bindir "${DESTDIR}${outdir}/")
 

Nit: trailing slash shouldn't be there.



Comment at: llvm/tools/llvm-config/llvm-config.cpp:361
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_INCLUDEDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);

Why the temporary `StringRef`?  Can you not just initialize `Path` with the 
literal?



Comment at: llvm/tools/llvm-config/llvm-config.cpp:366
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_BINDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);

Similar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810

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

[PATCH] D105049: [NFC] Remove extra semicolons in clang/lib/APINotes/APINotesFormat.h

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Thanks for the cleanup!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105049

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

[PATCH] D105368: Lex: add a callback for `#pragma mark`

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Correct lifetime of data in the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105368

Files:
  clang/include/clang/Lex/PPCallbacks.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/Pragma.cpp
  clang/unittests/Lex/PPCallbacksTest.cpp

Index: clang/unittests/Lex/PPCallbacksTest.cpp
===
--- clang/unittests/Lex/PPCallbacksTest.cpp
+++ clang/unittests/Lex/PPCallbacksTest.cpp
@@ -112,6 +112,20 @@
   unsigned State;
 };
 
+class PragmaMarkCallbacks : public PPCallbacks {
+public:
+  struct Mark {
+SourceLocation Location;
+std::string Trivia;
+  };
+
+  std::vector Marks;
+
+  void PragmaMark(SourceLocation Loc, StringRef Trivia) override {
+Marks.emplace_back(Mark{Loc, Trivia.str()});
+  }
+};
+
 // PPCallbacks test fixture.
 class PPCallbacksTest : public ::testing::Test {
 protected:
@@ -256,6 +270,36 @@
 return Callbacks->Results;
   }
 
+  std::vector
+  PragmaMarkCall(const char *SourceText) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText, "test.c");
+SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(SourceBuf)));
+
+HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+Diags, LangOpts, Target.get());
+TrivialModuleLoader ModLoader;
+
+Preprocessor PP(std::make_shared(), Diags, LangOpts,
+SourceMgr, HeaderInfo, ModLoader, /*IILookup=*/nullptr,
+/*OwnsHeaderSearch=*/false);
+PP.Initialize(*Target);
+
+auto *Callbacks = new PragmaMarkCallbacks;
+PP.addPPCallbacks(std::unique_ptr(Callbacks));
+
+// Lex source text.
+PP.EnterMainSourceFile();
+while (true) {
+  Token Tok;
+  PP.Lex(Tok);
+  if (Tok.is(tok::eof))
+break;
+}
+
+return Callbacks->Marks;
+  }
+
   PragmaOpenCLExtensionCallbacks::CallbackParameters
   PragmaOpenCLExtensionCall(const char *SourceText) {
 LangOptions OpenCLLangOpts;
@@ -424,6 +468,24 @@
   ASSERT_EQ(ExpectedState, Parameters.State);
 }
 
+TEST_F(PPCallbacksTest, CollectMarks) {
+  const char *Source =
+"#pragma mark\n"
+"#pragma mark\r\n"
+"#pragma mark - trivia\n"
+"#pragma mark - trivia\r\n";
+
+  auto Marks = PragmaMarkCall(Source);
+
+  ASSERT_EQ(4u, Marks.size());
+  ASSERT_TRUE(Marks[0].Trivia.empty());
+  ASSERT_TRUE(Marks[1].Trivia.empty());
+  ASSERT_FALSE(Marks[2].Trivia.empty());
+  ASSERT_FALSE(Marks[3].Trivia.empty());
+  ASSERT_EQ(" - trivia", Marks[2].Trivia);
+  ASSERT_EQ(" - trivia", Marks[3].Trivia);
+}
+
 TEST_F(PPCallbacksTest, DirectiveExprRanges) {
   const auto &Results1 = DirectiveExprRange("#if FLUZZY_FLOOF\n#endif\n");
   EXPECT_EQ(Results1.size(), 1U);
Index: clang/lib/Lex/Pragma.cpp
===
--- clang/lib/Lex/Pragma.cpp
+++ clang/lib/Lex/Pragma.cpp
@@ -412,9 +412,13 @@
   HeaderInfo.MarkFileIncludeOnce(getCurrentFileLexer()->getFileEntry());
 }
 
-void Preprocessor::HandlePragmaMark() {
+void Preprocessor::HandlePragmaMark(Token &MarkTok) {
   assert(CurPPLexer && "No current lexer?");
-  CurLexer->ReadToEndOfLine();
+
+  SmallString<64> Buffer;
+  CurLexer->ReadToEndOfLine(&Buffer);
+  if (Callbacks)
+Callbacks->PragmaMark(MarkTok.getLocation(), Buffer);
 }
 
 /// HandlePragmaPoison - Handle \#pragma GCC poison.  PoisonTok is the 'poison'.
@@ -992,7 +996,7 @@
 
   void HandlePragma(Preprocessor &PP, PragmaIntroducer Introducer,
 Token &MarkTok) override {
-PP.HandlePragmaMark();
+PP.HandlePragmaMark(MarkTok);
   }
 };
 
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2365,7 +2365,7 @@
 
 public:
   void HandlePragmaOnce(Token &OnceTok);
-  void HandlePragmaMark();
+  void HandlePragmaMark(Token &MarkTok);
   void HandlePragmaPoison();
   void HandlePragmaSystemHeader(Token &SysHeaderTok);
   void HandlePragmaDependency(Token &DependencyTok);
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -191,6 +191,10 @@
  StringRef Str) {
   }
 
+  /// Callback invoked when a \#pragma mark comment is read.
+  virtual void PragmaMark(SourceLocation Loc, StringRef Trivia) {
+  }
+
   /// Callback invoked when a \#pragma detect_mismatch directive is
   /// read.
   virtual void PragmaDetectMismatch(SourceLocation Loc, StringRef Name,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105368: Lex: add a callback for `#pragma mark`

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added reviewers: dgoldman, rsmith, aaron.ballman.
Herald added subscribers: usaxena95, kadircet, kbarton, nemanjai.
compnerd requested review of this revision.
Herald added a subscriber: ilya-biryukov.
Herald added a project: clang.

Allow a preprocessor observer to be notified of mark pragmas.  Although
this does not impact code generation in any way, it is useful for other
clients, such as clangd, to be able to identify any marked regions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105368

Files:
  clang/include/clang/Lex/PPCallbacks.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Lex/Pragma.cpp
  clang/unittests/Lex/PPCallbacksTest.cpp

Index: clang/unittests/Lex/PPCallbacksTest.cpp
===
--- clang/unittests/Lex/PPCallbacksTest.cpp
+++ clang/unittests/Lex/PPCallbacksTest.cpp
@@ -112,6 +112,20 @@
   unsigned State;
 };
 
+class PragmaMarkCallbacks : public PPCallbacks {
+public:
+  struct Mark {
+SourceLocation Location;
+StringRef Trivia;
+  };
+
+  std::vector Marks;
+
+  void PragmaMark(SourceLocation Loc, StringRef Trivia) override {
+Marks.emplace_back(Mark{Loc, Trivia});
+  }
+};
+
 // PPCallbacks test fixture.
 class PPCallbacksTest : public ::testing::Test {
 protected:
@@ -256,6 +270,36 @@
 return Callbacks->Results;
   }
 
+  std::vector
+  PragmaMarkCall(const char *SourceText) {
+std::unique_ptr SourceBuf =
+llvm::MemoryBuffer::getMemBuffer(SourceText, "test.c");
+SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(SourceBuf)));
+
+HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+Diags, LangOpts, Target.get());
+TrivialModuleLoader ModLoader;
+
+Preprocessor PP(std::make_shared(), Diags, LangOpts,
+SourceMgr, HeaderInfo, ModLoader, /*IILookup=*/nullptr,
+/*OwnsHeaderSearch=*/false);
+PP.Initialize(*Target);
+
+auto *Callbacks = new PragmaMarkCallbacks;
+PP.addPPCallbacks(std::unique_ptr(Callbacks));
+
+// Lex source text.
+PP.EnterMainSourceFile();
+while (true) {
+  Token Tok;
+  PP.Lex(Tok);
+  if (Tok.is(tok::eof))
+break;
+}
+
+return Callbacks->Marks;
+  }
+
   PragmaOpenCLExtensionCallbacks::CallbackParameters
   PragmaOpenCLExtensionCall(const char *SourceText) {
 LangOptions OpenCLLangOpts;
@@ -424,6 +468,24 @@
   ASSERT_EQ(ExpectedState, Parameters.State);
 }
 
+TEST_F(PPCallbacksTest, CollectMarks) {
+  const char *Source =
+"#pragma mark\n"
+"#pragma mark\r\n"
+"#pragma mark - trivia\n"
+"#pragma mark - trivia\r\n";
+
+  auto Marks = PragmaMarkCall(Source);
+
+  ASSERT_EQ(4u, Marks.size());
+  ASSERT_TRUE(Marks[0].Trivia.empty());
+  ASSERT_TRUE(Marks[1].Trivia.empty());
+  ASSERT_FALSE(Marks[2].Trivia.empty());
+  ASSERT_FALSE(Marks[3].Trivia.empty());
+  ASSERT_EQ(" - trivia", Marks[2].Trivia);
+  ASSERT_EQ(" - trivia", Marks[3].Trivia);
+}
+
 TEST_F(PPCallbacksTest, DirectiveExprRanges) {
   const auto &Results1 = DirectiveExprRange("#if FLUZZY_FLOOF\n#endif\n");
   EXPECT_EQ(Results1.size(), 1U);
Index: clang/lib/Lex/Pragma.cpp
===
--- clang/lib/Lex/Pragma.cpp
+++ clang/lib/Lex/Pragma.cpp
@@ -412,9 +412,13 @@
   HeaderInfo.MarkFileIncludeOnce(getCurrentFileLexer()->getFileEntry());
 }
 
-void Preprocessor::HandlePragmaMark() {
+void Preprocessor::HandlePragmaMark(Token &MarkTok) {
   assert(CurPPLexer && "No current lexer?");
-  CurLexer->ReadToEndOfLine();
+
+  SmallString<64> Buffer;
+  CurLexer->ReadToEndOfLine(&Buffer);
+  if (Callbacks)
+Callbacks->PragmaMark(MarkTok.getLocation(), Buffer);
 }
 
 /// HandlePragmaPoison - Handle \#pragma GCC poison.  PoisonTok is the 'poison'.
@@ -992,7 +996,7 @@
 
   void HandlePragma(Preprocessor &PP, PragmaIntroducer Introducer,
 Token &MarkTok) override {
-PP.HandlePragmaMark();
+PP.HandlePragmaMark(MarkTok);
   }
 };
 
Index: clang/include/clang/Lex/Preprocessor.h
===
--- clang/include/clang/Lex/Preprocessor.h
+++ clang/include/clang/Lex/Preprocessor.h
@@ -2365,7 +2365,7 @@
 
 public:
   void HandlePragmaOnce(Token &OnceTok);
-  void HandlePragmaMark();
+  void HandlePragmaMark(Token &MarkTok);
   void HandlePragmaPoison();
   void HandlePragmaSystemHeader(Token &SysHeaderTok);
   void HandlePragmaDependency(Token &DependencyTok);
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -191,6 +191,10 @@
  StringRef Str) {
   }
 
+  /// Callback invoked when a \#pragma mark comment is read.
+  virtual void PragmaMark(SourceLocation Loc, StringRef Trivia) {
+  }
+
   /// Callback invoked 

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Interesting, are the logs from the runs available?  I have run the test ~1 
times locally and its been stable.  Perhaps the logs can show what is going on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88666

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

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

I was able to play around with this further yesterday evening.  You are correct 
- the issue is the load preventing the watcher thread from spinning up.  I was 
able to reproduce this issue and resolve it by adding in a synchronization 
point (boolean + mutex + condition variable) before returning the directory 
watcher to ensure that the RDC was setup.  I looked through all the previous 
failure cases as well as the one that I was finally able to reproduce - it is 
always the initial notification that we missed (because the thread took too 
long to come up).  In the process I did do a few minor alterations as well.  I 
would like to get additional testing over the weekend on the bots so people 
aren't impacted if there turns out to be some other subtle threading issue.  
However, running this in a tight loop locally seemed to be pretty stable 
(switching the builds to a SSD locally did help uncover the flakiness) so I am 
confident that this should be safe.  I'm happy to address any additional 
comments in post-commit review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88666

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

[PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

This is a pretty straightforward cleanup now, which adds additional 
functionality by deferring work to CMake.  There are a couple of minor points 
about inconsistent quoting but this seems good otherwise.




Comment at: clang-tools-extra/clang-doc/tool/CMakeLists.txt:26
 install(FILES ../assets/index.js
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT clang-doc)

Why are these quoted but other uses not?



Comment at: flang/CMakeLists.txt:442
   install(DIRECTORY include/flang
-DESTINATION include
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
 COMPONENT flang-headers

Why is this quoted but other uses not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

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

[PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/cmake/modules/AddClang.cmake:127
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})

Ericson2314 wrote:
> compnerd wrote:
> > Ericson2314 wrote:
> > > compnerd wrote:
> > > > For the initial change, Id leave this off.  `CMAKE_INSTALL_LIBDIR` 
> > > > sometimes already deals with the bit suffix, so you can end up with two 
> > > > instances of `32` or `64`.  I think that cleaning that up separately, 
> > > > while focusing on the details of cleaning up how to handle 
> > > > `LLVM_LIBDIR_SUFFIX` is the right thing to do.  The same applies to the 
> > > > rest of the patch.
> > > https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/GNUInstallDirs.cmake#L257
> > >  Hmm I see what you mean. So you are saying `s/${ 
> > > CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}/${ CMAKE_INSTALL_LIBDIR}/` 
> > > everywhere?
> > Yes, that is what I was referring to.  I'm suggesting that you do *not* 
> > make that change instead.  That needs a much more involved change to clean 
> > up the use of `${LLVM_LIBDIR_SUFFIX}`.  I think that this change is already 
> > extremely large as is, and folding more into it is not going to help.
> So you are saying II should back all of these out into 
> `lib${LLVM_LIBDIR_SUFFIX}` as they were before, for now?
> 
> Yes I don't want to make this bigger either, and would rather be on the hook 
> for follow-up work than have this one be too massive to get over the finish 
> line.
Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

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

[PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/cmake/modules/AddClang.cmake:127
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})

Ericson2314 wrote:
> compnerd wrote:
> > For the initial change, Id leave this off.  `CMAKE_INSTALL_LIBDIR` 
> > sometimes already deals with the bit suffix, so you can end up with two 
> > instances of `32` or `64`.  I think that cleaning that up separately, while 
> > focusing on the details of cleaning up how to handle `LLVM_LIBDIR_SUFFIX` 
> > is the right thing to do.  The same applies to the rest of the patch.
> https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/GNUInstallDirs.cmake#L257
>  Hmm I see what you mean. So you are saying `s/${ 
> CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}/${ CMAKE_INSTALL_LIBDIR}/` 
> everywhere?
Yes, that is what I was referring to.  I'm suggesting that you do *not* make 
that change instead.  That needs a much more involved change to clean up the 
use of `${LLVM_LIBDIR_SUFFIX}`.  I think that this change is already extremely 
large as is, and folding more into it is not going to help.



Comment at: compiler-rt/cmake/base-config-ix.cmake:72
+  set(COMPILER_RT_INSTALL_PATH "" CACHE PATH
+"Prefix where built compiler-rt artifacts should be installed, comes 
before CMAKE_INSTALL_PREFIX.")
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit 
tests." OFF)

Ericson2314 wrote:
> compnerd wrote:
> > Please don't change the descriptions of the options as part of the 
> > `GNUInstallDirs` handling.  The change to `COMPILER_RT_INSTALL_PATH` looks 
> > incorrect.  Can you explain this change please?
> I tried explain this https://reviews.llvm.org/D99484#2655885 and the original 
> description about prefixes. The GNU install dir variables are allowed to be 
> absolute paths (and not necessary with the installation prefix) (and I needed 
> that for the NixOS patch), so always prepending `COMPILER_RT_INSTALL_PATH` as 
> is done doesn't work if that is `CMAKE_INSTALL_PREFIX` by default.
> 
> If you do `git grep '_PREFIX ""'` and also `git grep GRPC_INSTALL_PATH` you 
> will see that many similar variables also were already empty by default. I 
> agree this thing is a bit ugly, but by that argument I am not doing a new 
> hack for `GNUInstallDIrs` but rather applying an existing ideom consistently 
> in all packages.
Sure, but the *descriptions* of the options are needed to changed for changing 
the value.

That said, I'm not convinced that this change is okay.  It changes the way that 
compiler-rt can be built and used when building a toolchain image.  The 
handling of the install prefix in compiler-rt is significantly different from 
the other parts of LLVM, and so, I think that it may need to be done as a 
separate larger effort.



Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
 

Ericson2314 wrote:
> compnerd wrote:
> > Does this need to come here?  Why not push this to after the if block 
> > completes?  The same applies through out the rest of the patch.
> It might be fine here. But I was worried that in some of these cases code 
> included in those blocks might refer to the `GNUInstallDirs` variables.
> 
> Originally I had `GNUInstallDirs` only included in the conditional block 
> after `project(...)`, but then the combined build test failed because nothing 
> including `GnuInstallDirs` in that case. If there is an "entrypoint" 
> CMakeLists boilerplate that combined builds should always use, I think the 
> best thing would be to change it back and then additionally put 
> `GNUInstallDirs` there.
Unified builds always use `llvm/CMakeLists.txt`.  However, both should continue 
to work, and so you will need to add this in the subprojects as well.



Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:66
   install(FILES "${LIBCXX_BINARY_INCLUDE_DIR}/${fpath}"
-DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1/${dstdir}
+DESTINATION 
${LIBCXX_INSTALL_HEADER_PREFIX}${CMAKE_INSTALL_INCLUDEDIR}/c++/v1/${dstdir}
 COMPONENT cxx-headers

Ericson2314 wrote:
> compnerd wrote:
> > @ldionne - how is the `LIBCXX_INSTALL_HEADER_PREFIX` used?  Can altering 
> > the value of `CMAKE_INSTALL_INCLUDEDIR` serve the purpose?
> It is sometimes modified to be per target when multiple targets are being 
> used at once. All things `CMAKE_INSTALL_*` are globally scoped so in general 
> the combination builds are quite awkward.
> 
> (Having worked on Meson, I am really missing 
> https://mesonbuild.com/Subprojects.html which is exactly what's needed to do 
> this without these bespoke idioms that never work well enough . Alas...)
I don't think that bringing up other build systems is par