[PATCH] D151388: [HWASan] use hwasan linker for Android 14+

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151388

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

[PATCH] D149215: [MemProf] Control availability of hot/cold operator new from LTO link

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D149215#4303197 , @tejohnson wrote:

> In D149215#4303149 , @pcc wrote:
>
>>> Adds an LTO option
>>
>> Usual question: does it need to be an option? Could the allocator expose a 
>> symbol such as `__malloc_hot_cold` that the linker could check for in the 
>> symbol table?
>
> I thought about doing something like that, but the disadvantage is that it 
> requires support in the linkers (presumably at least both lld and the gold 
> plugin), instead of being centralized in LTO itself. That being said, I do 
> see existing lld code that currently looks for certain special __* symbols, 
> so maybe this would be ok. I agree it would be nice to have something 
> automatic, at least longer term. However, I think we need the internal option 
> anyway, for testing (especially via opt for simulating regular LTO). What do 
> you think of my adding a TODO to investigate the approach you are suggesting 
> here?

That's fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149215

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

[PATCH] D149215: [MemProf] Control availability of hot/cold operator new from LTO link

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

> Adds an LTO option

Usual question: does it need to be an option? Could the allocator expose a 
symbol such as `__malloc_hot_cold` that the linker could check for in the 
symbol table?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149215

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

[PATCH] D129700: [clang] Don't emit type tests for dllexport/import classes

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129700

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

[PATCH] D146497: libclang: Pass Clang install directory to driver via argv[0].

[Peter Collingbourne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG201fdef40dd6: libclang: Pass Clang install directory to 
driver via argv[0]. (authored by pcc).

Changed prior to commit:
  https://reviews.llvm.org/D146497?vs=506832=507525#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146497

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/test/Index/record-completion-invocation.c
  clang/test/Index/record-parsing-invocation.c
  clang/tools/libclang/CIndex.cpp


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -4013,8 +4013,17 @@
 struct CXUnsavedFile *unsaved_files, unsigned num_unsaved_files,
 unsigned options, CXTranslationUnit *out_TU) {
   noteBottomOfStack();
+
+  if (!CIdx)
+return CXError_InvalidArguments;
+
+  SmallString<64> ClangPath(
+  static_cast(CIdx)->getClangToolchainPath());
+  llvm::sys::path::append(ClangPath, "bin");
+  llvm::sys::path::append(ClangPath, "clang");
+
   SmallVector Args;
-  Args.push_back("clang");
+  Args.push_back(ClangPath.c_str());
   Args.append(command_line_args, command_line_args + num_command_line_args);
   return clang_parseTranslationUnit2FullArgv(
   CIdx, source_filename, Args.data(), Args.size(), unsaved_files,
Index: clang/test/Index/record-parsing-invocation.c
===
--- clang/test/Index/record-parsing-invocation.c
+++ clang/test/Index/record-parsing-invocation.c
@@ -25,5 +25,5 @@
 #  pragma clang __debug parser_crash
 #endif
 
-// CHECK: 
{"toolchain":"{{.*}}","libclang.operation":"parse","libclang.opts":1,"args":["clang","-fno-spell-checking","{{.*}}record-parsing-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"]}
-// CHECK-UNSAVED: 
{"toolchain":"{{.*}}","libclang.operation":"parse","libclang.opts":1,"args":["clang","-fno-spell-checking","{{.*}}record-parsing-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"],"unsaved_file_hashes":[{"name":"{{.*}}record-parsing-invocation.c","md5":"aee23773de90e665992b48209351d70e"}]}
+// CHECK: 
{"toolchain":"{{.*}}","libclang.operation":"parse","libclang.opts":1,"args":["{{.*}}bin{{.*}}clang","-fno-spell-checking","{{.*}}record-parsing-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"]}
+// CHECK-UNSAVED: 
{"toolchain":"{{.*}}","libclang.operation":"parse","libclang.opts":1,"args":["{{.*}}bin{{.*}}clang","-fno-spell-checking","{{.*}}record-parsing-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"],"unsaved_file_hashes":[{"name":"{{.*}}record-parsing-invocation.c","md5":"aee23773de90e665992b48209351d70e"}]}
Index: clang/test/Index/record-completion-invocation.c
===
--- clang/test/Index/record-completion-invocation.c
+++ clang/test/Index/record-completion-invocation.c
@@ -9,4 +9,4 @@
 // RUN: env LIBCLANG_DISABLE_CRASH_RECOVERY=1 
CINDEXTEST_INVOCATION_EMISSION_PATH=%t not --crash c-index-test 
-code-completion-at=%s:10:1 
"-remap-file=%s,%S/Inputs/record-parsing-invocation-remap.c" %s
 // RUN: cat %t/libclang-* | FileCheck %s
 
-// CHECK: 
{"toolchain":"{{.*}}","libclang.operation":"complete","libclang.opts":1,"args":["clang","-fno-spell-checking","{{.*}}record-completion-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"],"invocation-args":["-code-completion-at={{.*}}record-completion-invocation.c:10:1"],"unsaved_file_hashes":[{"name":"{{.*}}record-completion-invocation.c","md5":"aee23773de90e665992b48209351d70e"}]}
+// CHECK: 
{"toolchain":"{{.*}}","libclang.operation":"complete","libclang.opts":1,"args":["{{.*}}bin{{.*}}clang","-fno-spell-checking","{{.*}}record-completion-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"],"invocation-args":["-code-completion-at={{.*}}record-completion-invocation.c:10:1"],"unsaved_file_hashes":[{"name":"{{.*}}record-completion-invocation.c","md5":"aee23773de90e665992b48209351d70e"}]}
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -899,8 +899,13 @@
 
 /**
  * Same as clang_parseTranslationUnit2 but requires a full command line
- * for \c command_line_args including argv[0]. This is useful if the standard
- * library paths are relative to the binary.
+ * for \c command_line_args including argv[0].
+ *
+ * This is useful if the driver uses paths relative to the binary and either
+ * you are targeting libclang versions older than Clang 17, or libclang is
+ * installed to a non-standard location. Clang 17 and newer will 

[PATCH] D146497: libclang: Pass Clang install directory to driver via argv[0].

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/tools/libclang/CIndex.cpp:4022
+  llvm::sys::path::append(ClangPath, "bin");
+  llvm::sys::path::append(ClangPath, "clang");
+

pcc wrote:
> aaron.ballman wrote:
> > I suspect this doesn't matter *too* much, but... on Windows, wouldn't this 
> > be `clang.exe` instead? (This isn't new to your patch, so mostly wondering 
> > if there's a separate issue we should track.)
> I don't think it matters; the `Driver` class strips off the `bin/clang` part 
> before storing it:
> https://github.com/llvm/llvm-project/blob/4d18d97b594ccaa3cbd79beb4afef45e4156dc8d/clang/lib/Driver/Driver.cpp#L211
> 
> Aside from that, the only user I could find that's reachable from this 
> function is a rather obscure codegen feature:
> https://github.com/llvm/llvm-project/blob/fe27495be2040007c7b20844a9371b06156ab405/clang/lib/Frontend/CompilerInvocation.cpp#L4464
> and this API doesn't do codegen.
And of course I link to a line of code that stores the filename somewhere, 
contradicting what I wrote... that `Name` field, though, seems to be only used 
for crash reporting and a help message:

https://github.com/llvm/llvm-project/blob/4d18d97b594ccaa3cbd79beb4afef45e4156dc8d/clang/lib/Driver/Driver.cpp#L1516
https://github.com/llvm/llvm-project/blob/4d18d97b594ccaa3cbd79beb4afef45e4156dc8d/clang/lib/Driver/Driver.cpp#L1801
https://github.com/llvm/llvm-project/blob/4d18d97b594ccaa3cbd79beb4afef45e4156dc8d/clang/lib/Driver/Driver.cpp#L1931


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146497

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

[PATCH] D146497: libclang: Pass Clang install directory to driver via argv[0].

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/tools/libclang/CIndex.cpp:4022
+  llvm::sys::path::append(ClangPath, "bin");
+  llvm::sys::path::append(ClangPath, "clang");
+

aaron.ballman wrote:
> I suspect this doesn't matter *too* much, but... on Windows, wouldn't this be 
> `clang.exe` instead? (This isn't new to your patch, so mostly wondering if 
> there's a separate issue we should track.)
I don't think it matters; the `Driver` class strips off the `bin/clang` part 
before storing it:
https://github.com/llvm/llvm-project/blob/4d18d97b594ccaa3cbd79beb4afef45e4156dc8d/clang/lib/Driver/Driver.cpp#L211

Aside from that, the only user I could find that's reachable from this function 
is a rather obscure codegen feature:
https://github.com/llvm/llvm-project/blob/fe27495be2040007c7b20844a9371b06156ab405/clang/lib/Frontend/CompilerInvocation.cpp#L4464
and this API doesn't do codegen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146497

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

[PATCH] D146497: libclang: Pass Clang install directory to driver via argv[0].

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: bkramer, aaron.ballman.
Herald added subscribers: mikhail.ramalho, arphaman.
Herald added a project: All.
pcc requested review of this revision.
Herald added a project: clang.

Various driver features, such as the sysroot path detection for Android
targets, rely on being able to find the Clang install directory (look
for callers of `getDriver().getInstalledDir()`). However, the install
directory isn't currently being plumbed through to the driver, which is
conventionally done via the argv[0] passed to the Driver constructor.

It looks like D14695  attempted to fix this by 
adding another API that
allows specifying the argv[0]. However, rather than requiring every
user of libclang to switch to this API for correct behavior, let's have
the other existing APIs work by default, by using the existing logic in
libclang for finding the install directory.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146497

Files:
  clang/include/clang-c/Index.h
  clang/test/Index/record-completion-invocation.c
  clang/test/Index/record-parsing-invocation.c
  clang/tools/libclang/CIndex.cpp


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -4013,8 +4013,16 @@
 struct CXUnsavedFile *unsaved_files, unsigned num_unsaved_files,
 unsigned options, CXTranslationUnit *out_TU) {
   noteBottomOfStack();
+
+  if (!CIdx)
+return CXError_InvalidArguments;
+  SmallString<64> ClangPath(
+  static_cast(CIdx)->getClangToolchainPath());
+  llvm::sys::path::append(ClangPath, "bin");
+  llvm::sys::path::append(ClangPath, "clang");
+
   SmallVector Args;
-  Args.push_back("clang");
+  Args.push_back(ClangPath.c_str());
   Args.append(command_line_args, command_line_args + num_command_line_args);
   return clang_parseTranslationUnit2FullArgv(
   CIdx, source_filename, Args.data(), Args.size(), unsaved_files,
Index: clang/test/Index/record-parsing-invocation.c
===
--- clang/test/Index/record-parsing-invocation.c
+++ clang/test/Index/record-parsing-invocation.c
@@ -25,5 +25,5 @@
 #  pragma clang __debug parser_crash
 #endif
 
-// CHECK: 
{"toolchain":"{{.*}}","libclang.operation":"parse","libclang.opts":1,"args":["clang","-fno-spell-checking","{{.*}}record-parsing-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"]}
-// CHECK-UNSAVED: 
{"toolchain":"{{.*}}","libclang.operation":"parse","libclang.opts":1,"args":["clang","-fno-spell-checking","{{.*}}record-parsing-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"],"unsaved_file_hashes":[{"name":"{{.*}}record-parsing-invocation.c","md5":"aee23773de90e665992b48209351d70e"}]}
+// CHECK: 
{"toolchain":"{{.*}}","libclang.operation":"parse","libclang.opts":1,"args":["{{.*}}bin{{.*}}clang","-fno-spell-checking","{{.*}}record-parsing-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"]}
+// CHECK-UNSAVED: 
{"toolchain":"{{.*}}","libclang.operation":"parse","libclang.opts":1,"args":["{{.*}}bin{{.*}}clang","-fno-spell-checking","{{.*}}record-parsing-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"],"unsaved_file_hashes":[{"name":"{{.*}}record-parsing-invocation.c","md5":"aee23773de90e665992b48209351d70e"}]}
Index: clang/test/Index/record-completion-invocation.c
===
--- clang/test/Index/record-completion-invocation.c
+++ clang/test/Index/record-completion-invocation.c
@@ -9,4 +9,4 @@
 // RUN: env LIBCLANG_DISABLE_CRASH_RECOVERY=1 
CINDEXTEST_INVOCATION_EMISSION_PATH=%t not --crash c-index-test 
-code-completion-at=%s:10:1 
"-remap-file=%s,%S/Inputs/record-parsing-invocation-remap.c" %s
 // RUN: cat %t/libclang-* | FileCheck %s
 
-// CHECK: 
{"toolchain":"{{.*}}","libclang.operation":"complete","libclang.opts":1,"args":["clang","-fno-spell-checking","{{.*}}record-completion-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"],"invocation-args":["-code-completion-at={{.*}}record-completion-invocation.c:10:1"],"unsaved_file_hashes":[{"name":"{{.*}}record-completion-invocation.c","md5":"aee23773de90e665992b48209351d70e"}]}
+// CHECK: 
{"toolchain":"{{.*}}","libclang.operation":"complete","libclang.opts":1,"args":["{{.*}}bin{{.*}}clang","-fno-spell-checking","{{.*}}record-completion-invocation.c","-Xclang","-detailed-preprocessing-record","-fallow-editor-placeholders"],"invocation-args":["-code-completion-at={{.*}}record-completion-invocation.c:10:1"],"unsaved_file_hashes":[{"name":"{{.*}}record-completion-invocation.c","md5":"aee23773de90e665992b48209351d70e"}]}
Index: clang/include/clang-c/Index.h

[PATCH] D129700: [clang] Don't emit type tests for dllexport/import classes

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Can't the code in CodeGenModule::HasHiddenLTOVisibility be moved here instead 
of duplicating it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129700

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

[PATCH] D144035: [SCEV] Ensure SCEV does not replace aliases with their aliasees

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144035

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

[PATCH] D144035: [hwasan] Ensure hwasan aliases do not have ODR linkage

[Peter Collingbourne via Phabricator via cfe-commits]

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

Passes shouldn't be replacing aliases with aliasees in general, see D66606 
. The right fix is to fix SCEV to not do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144035

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

[PATCH] D143634: [ModuleUtils] Assert correct linkage and visibility of structors' COMDAT key

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

I've always considered that this should be fixed by extending the 
resolution-based LTO API to also include resolutions for comdats.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143634

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

[PATCH] D139395: Add CFI integer types normalization

[Peter Collingbourne via Phabricator via cfe-commits]

pcc accepted this revision.
pcc added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139395

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

[PATCH] D139395: Add CFI integer types normalization

[Peter Collingbourne via Phabricator via cfe-commits]

pcc accepted this revision.
pcc added a comment.

LGTM with nits




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1694
+  getCXXABI().getMangleContext().mangleTypeName(
+  T, Out, !!getCodeGenOpts().SanitizeCfiICallNormalizeIntegers);
+

Is the !! necessary here?



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6881
+getCXXABI().getMangleContext().mangleTypeName(
+T, Out, !!getCodeGenOpts().SanitizeCfiICallNormalizeIntegers);
+

Likewise



Comment at: clang/test/CodeGen/cfi-icall-normalize.c:55-75
+// CHECK: ![[TYPE0]] = !{i64 0, !"_ZTSFv{{u2i8|u2u8}}E.normalized"}
+// CHECK: ![[TYPE1]] = !{i64 0, !"_ZTSFv{{u2i8S_|u2u8u2i8}}E.normalized"}
+// CHECK: ![[TYPE2]] = !{i64 0, !"_ZTSFv{{u2i8S_S_|u2u8u2i8S0_}}E.normalized"}
+// CHECK: ![[TYPE3]] = !{i64 0, !"_ZTSFv{{u3i16|u3i32|u3i64}}E.normalized"}
+// CHECK: ![[TYPE4]] = !{i64 0, !"_ZTSFv{{u3i16|u3i32|u3i64}}S_E.normalized"}
+// CHECK: ![[TYPE5]] = !{i64 0, !"_ZTSFv{{u3i16|u3i32|u3i64}}S_S_E.normalized"}
+// CHECK: ![[TYPE6]] = !{i64 0, !"_ZTSFv{{u3i32|u3i64}}E.normalized"}

Shouldn't these all be checking for specific types? Since you're specifying a 
triple, the width and signedness of the integer types are fixed.



Comment at: clang/test/CodeGen/cfi-icall-normalize2.c:9
+// CHECK-SAME: {{.*}}!type ![[TYPE1:[0-9]+]] !type !{{[0-9]+}}
+// CHECK: call i1 @llvm.type.test({{i8\*|ptr}} {{%f|%0}}, metadata 
!"_ZTSFv{{u3i16|u3i32|u3i64}}E.normalized")
+fn(arg);

Likewise; also below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139395

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

[PATCH] D139395: Add CFI integer types normalization

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D139395#4066948 , @samitolvanen 
wrote:

> Thanks for the patch, Ramon. This looks like a reasonable approach to me, and 
> just for reference, here appears to be the corresponding rustc change:
>
> https://github.com/rust-lang/rust/pull/105452/commits/9087c336103d0fa0b465acf8dbc1e4651250fb05
>
> @pcc did you have any other concerns about adding this option?

I discussed this out of band with Ramon and we agreed that the new option 
should be marked as experimental because the rustc implementation is not yet 
finalized. I think that the criteria for removing the experimental marking 
should be that there is a full implementation of integer normalization for Rust 
(or some other language) that has been tested against a large codebase that 
uses FFI. Aside from that I have no further concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139395

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

[PATCH] D139395: Add CFI integer types normalization

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D139395#3990772 , @rcvalle wrote:

> I elaborated on the reasons why not use a generalized encoding in the design 
> document in the tracking issue 
> https://github.com/rust-lang/rust/issues/89653. The tl;dr; is that it will 
> result in less comprehensive protection by either using a generalized 
> encoding for all C and C++ -compiled code or across the FFI boundary, and 
> will degrade the security of the program when linking foreign Rust-compiled 
> code into a program written in C or C++ because the program previously used a 
> more comprehensive encoding for all its compiled code, not fixing the issue 
> described in the design document and the RFC 
> https://github.com/rcvalle/rfcs/blob/improve-c-types-for-cross-language-cfi/text/-improve-c-types-for-cross-language-cfi.md#appendix.

Ack.




Comment at: clang/lib/AST/ItaniumMangle.cpp:2952
+  // uu
+  if (NormalizeIntegers && T->isInteger()) {
+if (T->isSignedInteger()) {

rcvalle wrote:
> pcc wrote:
> > `isInteger()` will return true for enums, but only if they are complete. 
> > This would mean that code such as
> > ```
> > void (*f)(enum E *e);
> > 
> > void g() {
> >   f(0);
> > }
> > ```
> > would use a different encoding to call `f` depending on whether the TU 
> > completes the enum `E`, if pointee types are considered part of the 
> > encoding.
> Isn't `isIntegerType()` that does that? `isInteger()` definition is:
> 
> ```
>   bool isInteger() const {
> return getKind() >= Bool && getKind() <= Int128;
>   }
> ```
Ah yes, sorry, somehow I read this as a call to `isIntegerType()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139395

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

[PATCH] D139395: Add CFI integer types normalization

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a reviewer: pcc.
pcc added a comment.

A high level question is whether we want to base the cross-language encoding on 
Itanium at all. Itanium has concepts such as substitutions that will complicate 
the implementation in other languages. Encoding pointee types can also lead to 
complications in cross-language encodings.

It may be better to consider developing a custom encoding. For the encoding 
being prototyped for the pointer authentication ABI on Android, the Rust side 
of the implementation is very simple:

https://github.com/pcc/rust/blob/d37ad119171635219ff21e054780d31024d24200/compiler/rustc_ty_utils/src/abi.rs#L396




Comment at: clang/lib/AST/ItaniumMangle.cpp:2952
+  // uu
+  if (NormalizeIntegers && T->isInteger()) {
+if (T->isSignedInteger()) {

`isInteger()` will return true for enums, but only if they are complete. This 
would mean that code such as
```
void (*f)(enum E *e);

void g() {
  f(0);
}
```
would use a different encoding to call `f` depending on whether the TU 
completes the enum `E`, if pointee types are considered part of the encoding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139395

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

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D138337#3972138 , @samitolvanen 
wrote:

> In D138337#3972009 , @pcc wrote:
>
>> Can't this be implicit if LTO is being used?
>
> I would prefer to keep this behind a flag (similarly to `-mibt-seal`), so we 
> can better control when and where the optimization enabled. For example, 
> enabling this implicitly would currently break LTO+KCFI kernel builds as the 
> kernel has global functions that are not address-taken in C code, but still 
> end up being indirectly called because their addresses were taken in assembly 
> code that wasn't visible to LTO (e.g. arm64's `alternative_cb` mechanism).

Isn't that just a bug in the optimization? It shouldn't be applying this to 
symbols where `VisibleToRegularObj` is set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138337

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

[PATCH] D138337: Add support for kcfi-seal optimization with LTO

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Can't this be implicit if LTO is being used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138337

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

[PATCH] D120862: Sema: Allow scoped enums as source type for integral conversion.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Does that DR apply retroactively to C++17? I get the impression that "Status: 
C++20" means that the issue was only fixed in C++20, which would make this 
well-formed with `-std=c++17`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120862

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

[PATCH] D135421: Driver: Change default Android linker to lld.

[Peter Collingbourne 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 rG8d9c4a7425d9: Driver: Change default Android linker to lld. 
(authored by pcc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135421

Files:
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h
  clang/test/Driver/coverage-ld.c
  clang/test/Driver/fuse-ld.c
  clang/test/Driver/sanitizer-ld.c

Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -169,7 +169,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID %s
 //
-// CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID: "-pie"
 // CHECK-ASAN-ANDROID-NOT: "-lc"
 // CHECK-ASAN-ANDROID-NOT: "-lpthread"
@@ -184,7 +184,7 @@
 // RUN: -static-libsan \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID-STATICLIBASAN %s
 //
-// CHECK-ASAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-ASAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-STATICLIBASAN: libclang_rt.asan-arm-android.a"
 // CHECK-ASAN-ANDROID-STATICLIBASAN-NOT: "-lpthread"
 // CHECK-ASAN-ANDROID-STATICLIBASAN-NOT: "-lrt"
@@ -195,7 +195,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-ANDROID %s
 //
-// CHECK-UBSAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-UBSAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-UBSAN-ANDROID: "-pie"
 // CHECK-UBSAN-ANDROID-NOT: "-lc"
 // CHECK-UBSAN-ANDROID-NOT: "-lpthread"
@@ -210,7 +210,7 @@
 // RUN: -static-libsan \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-ANDROID-STATICLIBASAN %s
 //
-// CHECK-UBSAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-UBSAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-UBSAN-ANDROID-STATICLIBASAN: libclang_rt.ubsan_standalone-arm-android.a"
 // CHECK-UBSAN-ANDROID-STATICLIBASAN-NOT: "-lpthread"
 // CHECK-UBSAN-ANDROID-STATICLIBASAN-NOT: "-lrt"
@@ -222,7 +222,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID-X86 %s
 //
-// CHECK-ASAN-ANDROID-X86: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-ASAN-ANDROID-X86: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-X86: "-pie"
 // CHECK-ASAN-ANDROID-X86-NOT: "-lc"
 // CHECK-ASAN-ANDROID-X86-NOT: "-lpthread"
@@ -245,7 +245,7 @@
 // RUN: -shared \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID-SHARED %s
 //
-// CHECK-ASAN-ANDROID-SHARED: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-ASAN-ANDROID-SHARED: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-SHARED-NOT: "-lc"
 // CHECK-ASAN-ANDROID-SHARED: libclang_rt.asan-arm-android.so"
 // CHECK-ASAN-ANDROID-SHARED-NOT: "-lpthread"
@@ -760,7 +760,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-SAFESTACK-ANDROID-ARM %s
 //
-// CHECK-SAFESTACK-ANDROID-ARM: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SAFESTACK-ANDROID-ARM: "{{(.*[^-.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-SAFESTACK-ANDROID-ARM-NOT: libclang_rt.safestack
 
 // RUN: %clang -### %s -shared 2>&1 \
@@ -768,7 +768,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-SAFESTACK-SHARED-ANDROID-ARM %s
 //
-// CHECK-SAFESTACK-SHARED-ANDROID-ARM: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SAFESTACK-SHARED-ANDROID-ARM: "{{(.*[^-.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-SAFESTACK-SHARED-ANDROID-ARM-NOT: libclang_rt.safestack
 
 // RUN: %clang -### %s 2>&1 \
@@ -776,7 +776,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-SAFESTACK-ANDROID-AARCH64 %s
 //
-// CHECK-SAFESTACK-ANDROID-AARCH64: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SAFESTACK-ANDROID-AARCH64: "{{(.*[^-.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-SAFESTACK-ANDROID-AARCH64-NOT: libclang_rt.safestack
 
 // RUN: %clang -fsanitize=undefined -### %s 2>&1 \
@@ -891,7 +891,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-SCUDO-ANDROID %s
 //
-// CHECK-SCUDO-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SCUDO-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-SCUDO-ANDROID-NOT: "-lc"
 // CHECK-SCUDO-ANDROID: "-pie"
 // CHECK-SCUDO-ANDROID-NOT: "-lpthread"
@@ -905,7 +905,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN: -static-libsan \
 // 

[PATCH] D135421: Driver: Change default Android linker to lld.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135421

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

[PATCH] D135421: Driver: Change default Android linker to lld.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: srhines, rprichard, danalbert.
Herald added subscribers: danielkiss, cryptoad.
Herald added a project: All.
pcc requested review of this revision.
Herald added a subscriber: MaskRay.
Herald added a project: clang.

The clang distributed with the Android NDK has defaulted to lld since r22,
so let's update the driver to match.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135421

Files:
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h
  clang/test/Driver/coverage-ld.c
  clang/test/Driver/fuse-ld.c
  clang/test/Driver/sanitizer-ld.c

Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -169,7 +169,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID %s
 //
-// CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID: "-pie"
 // CHECK-ASAN-ANDROID-NOT: "-lc"
 // CHECK-ASAN-ANDROID-NOT: "-lpthread"
@@ -184,7 +184,7 @@
 // RUN: -static-libsan \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID-STATICLIBASAN %s
 //
-// CHECK-ASAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-ASAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-STATICLIBASAN: libclang_rt.asan-arm-android.a"
 // CHECK-ASAN-ANDROID-STATICLIBASAN-NOT: "-lpthread"
 // CHECK-ASAN-ANDROID-STATICLIBASAN-NOT: "-lrt"
@@ -195,7 +195,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-ANDROID %s
 //
-// CHECK-UBSAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-UBSAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-UBSAN-ANDROID: "-pie"
 // CHECK-UBSAN-ANDROID-NOT: "-lc"
 // CHECK-UBSAN-ANDROID-NOT: "-lpthread"
@@ -210,7 +210,7 @@
 // RUN: -static-libsan \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-ANDROID-STATICLIBASAN %s
 //
-// CHECK-UBSAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-UBSAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-UBSAN-ANDROID-STATICLIBASAN: libclang_rt.ubsan_standalone-arm-android.a"
 // CHECK-UBSAN-ANDROID-STATICLIBASAN-NOT: "-lpthread"
 // CHECK-UBSAN-ANDROID-STATICLIBASAN-NOT: "-lrt"
@@ -222,7 +222,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID-X86 %s
 //
-// CHECK-ASAN-ANDROID-X86: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-ASAN-ANDROID-X86: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-X86: "-pie"
 // CHECK-ASAN-ANDROID-X86-NOT: "-lc"
 // CHECK-ASAN-ANDROID-X86-NOT: "-lpthread"
@@ -245,7 +245,7 @@
 // RUN: -shared \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-ANDROID-SHARED %s
 //
-// CHECK-ASAN-ANDROID-SHARED: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-ASAN-ANDROID-SHARED: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-SHARED-NOT: "-lc"
 // CHECK-ASAN-ANDROID-SHARED: libclang_rt.asan-arm-android.so"
 // CHECK-ASAN-ANDROID-SHARED-NOT: "-lpthread"
@@ -760,7 +760,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-SAFESTACK-ANDROID-ARM %s
 //
-// CHECK-SAFESTACK-ANDROID-ARM: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SAFESTACK-ANDROID-ARM: "{{(.*[^-.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-SAFESTACK-ANDROID-ARM-NOT: libclang_rt.safestack
 
 // RUN: %clang -### %s -shared 2>&1 \
@@ -768,7 +768,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-SAFESTACK-SHARED-ANDROID-ARM %s
 //
-// CHECK-SAFESTACK-SHARED-ANDROID-ARM: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SAFESTACK-SHARED-ANDROID-ARM: "{{(.*[^-.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-SAFESTACK-SHARED-ANDROID-ARM-NOT: libclang_rt.safestack
 
 // RUN: %clang -### %s 2>&1 \
@@ -776,7 +776,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-SAFESTACK-ANDROID-AARCH64 %s
 //
-// CHECK-SAFESTACK-ANDROID-AARCH64: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SAFESTACK-ANDROID-AARCH64: "{{(.*[^-.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-SAFESTACK-ANDROID-AARCH64-NOT: libclang_rt.safestack
 
 // RUN: %clang -fsanitize=undefined -### %s 2>&1 \
@@ -891,7 +891,7 @@
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
 // RUN:   | FileCheck --check-prefix=CHECK-SCUDO-ANDROID %s
 //
-// CHECK-SCUDO-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
+// CHECK-SCUDO-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-SCUDO-ANDROID-NOT: "-lc"
 // CHECK-SCUDO-ANDROID: "-pie"
 // CHECK-SCUDO-ANDROID-NOT: "-lpthread"
@@ -905,7 +905,7 @@
 // RUN: 

[PATCH] D120862: Sema: Allow scoped enums as source type for integral conversion.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Ping^2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120862

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

[PATCH] D115844: [ubsan] Using metadata instead of prologue data for function sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM

> Yes, but not indirectly called from C/C++ but rather from compiler-generated 
> code right? That's presumably why this patch + D116130 
>  worked for @zhuhan0.

If the answer to my question is "yes" can you please update the commit message 
to not talk about changing the function signature since that's not the problem 
here.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1843
+
+  auto *FTRTTIProxy = new llvm::GlobalVariable(
+  TheModule, Addr->getType(),

ychen wrote:
> pcc wrote:
> > ychen wrote:
> > > pcc wrote:
> > > > Are these proxy variables necessary? I think that now that we have 
> > > > custom code generation for this you should be able to use a GOTPCREL 
> > > > relocation to refer to the global.
> > > I attempted the GOTPCREL approach in a local branch. It didn't work for a 
> > > reason that I couldn't remember off the top of my head. I'll find out.
> > > 
> > > > I think that now that we have custom code generation for this 
> > > 
> > > Sorry, I don't quite follow which `custom code generation` you are 
> > > referring to. Do you mean the changes in `AsmPrinter.cpp`?
> > > Sorry, I don't quite follow which custom code generation you are 
> > > referring to. Do you mean the changes in AsmPrinter.cpp?
> > 
> > Yes, that's what I meant.
> @pcc, I looked into my local branch that uses PCREL approach. It is not 
> simpler/cleaner than the current approach due to the X86/X86-64, macho/ELF 
> difference. These need their specific relocation types.
Okay, let's go with this then.



Comment at: llvm/docs/LangRef.rst:5209
 know or know it can't preserve. Currently there is an exception for metadata
 attachment to globals for ``!type`` and ``!absolute_symbol`` which can't be
 unconditionally dropped unless the global is itself deleted.

We should add `!func_sanitize` here as well.



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:843
+
+  // Emit the function prologue data for the indirect call sanitizer.
+  if (const MDNode *MD = F.getMetadata(LLVMContext::MD_func_sanitize)) {

ychen wrote:
> pcc wrote:
> > What if we have both prologue data and this metadata? Should it be an error?
> It may or may not be depending on what is in the prologue data (if it just 
> adds up a counter in the prologue, it should be fine?). How about clarifying 
> this in Langref for this new `MD_func_sanitize`? If being conservative, we 
> could error this out in the Verifier. WDYT?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115844

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

[PATCH] D119296: KCFI sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6694
 
-  if (isExternallyVisible(T->getLinkage())) {
+  if (isExternallyVisible(T->getLinkage()) || !OnlyExternal) {
 std::string OutName;

It would be better to have a separate function for computing the KCFI type ids 
than to try and reuse this one, since you don't need the MDStrings (i.e. 
unnecessary string uniquing) in your new caller. It also isn't appropriate to 
pass MetadataIdMap in your new caller because you'll end up with inconsistent 
values added to MetadataIdMap if we end up calling the other caller (e.g. if 
both CFI and KCFI are enabled), but that's moot if you avoid it.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2257
+
+  F->setPrefixData(CreateKCFITypeId(FD->getType()));
+  F->addFnAttr("kcfi-target");

samitolvanen wrote:
> ychen wrote:
> > FYI: using prefix data may not work for the C++ coroutine. 
> > (https://github.com/llvm/llvm-project/issues/49689) because corosplit pass 
> > may clone coro functions and change its function type. But prefix data is 
> > opaque, so there is no way to detect this, and then drop the prefix data in 
> > the corosplit pass.
> > 
> > If this is a concern for now or for the near future, it might be better to 
> > use alternative approaches like D115844.
> > FYI: using prefix data may not work for the C++ coroutine.
> 
> Thanks for the link, that's interesting. The Linux kernel doesn't use C++ so 
> this isn't a concern there, but I suppose there could theoretically also be 
> C++ users for this feature. @pcc, any thoughts if we should just switch from 
> prefix data to a metadata node here?
I think the issue with coroutines was more about the fact that the function was 
cloned (invalidating the relative reference in the prefix data) than that the 
function type was changed. I seem to recall from a discussion with the 
coroutine folks that the functions created by the coroutine pass with different 
types aren't actually intended to be called directly from C/C++, so the fact 
that the types are changed doesn't matter.

I think the main advantage of metadata here would be in doing things like the 
check for type mismatches in direct calls more easily, so it seems like a 
reasonable approach even though the rationale is different.



Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:2453
+  // additional machine instructions being emitted between the check and
+  // the call. This means we don't have to worry about expanding BLR_BTI
+  // and TCRETURNri* pseudos.

For PAuth ABI the motivation for avoiding the gap between check and call was 
around avoiding spilling the verified pointer, is this not possible with KCFI?



Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:119
+  MCSymbol *FnSym = OutContext.getOrCreateSymbol("__cfi_" + MF.getName());
+  // Use the same linkage as the parent function.
+  emitLinkage((), FnSym);

If this is just about satisfying objtool do these symbols need to be exported 
or can they just be STB_LOCAL?



Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83
+KCFI_NT_CALL,
+KCFI_TC_RETURN,
+

samitolvanen wrote:
> joaomoreira wrote:
> > samitolvanen wrote:
> > > joaomoreira wrote:
> > > > I did not revise the entire patch yet. With this said, IMHO, this looks 
> > > > like an overcomplication of a simple problem. Is there a reason why you 
> > > > really need specific KCFI_ nodes instead of only embedding the hash 
> > > > information into an attribute at the Machine Instruction? Then, if hash 
> > > > == 0, it just means it is a call that doesn't need instrumentation.
> > > > 
> > > > This latter approach will require less code and should be easier to 
> > > > maintain compatible with other CFI approaches. If the reason is because 
> > > > you don't want to have a useless attribute for non-call instructions, 
> > > > then you could possibly have a map where you bind the call instruction 
> > > > with a respective hash.
> > > > 
> > > > Unless there is a strong reason for these, I would much better prefer 
> > > > the slim approach suggested. Either way, if there is a reason for this, 
> > > > I would also suggest that you at least don't name these as 
> > > > "KCFI_something", as in the future others might want to reuse the same 
> > > > structure for other CFI approaches.
> > > > Is there a reason why you really need specific KCFI_ nodes instead of 
> > > > only embedding the hash information into an attribute at the Machine 
> > > > Instruction?
> > > 
> > > This implementation is similar to `CALL_RVMARKER`, `CALL_BTI` and 
> > > basically all other pseudo call instructions in LLVM. Is adding an 
> > > attribute to `MachineInstr` the preferred approach instead?
> > > 
> > > > I would also suggest that you at least don't name these as 
> > > > "KCFI_something", 

[PATCH] D127876: [clang] Don't emit type test/assume for virtual classes that should never participate in WPD

[Peter Collingbourne via Phabricator via cfe-commits]

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

Okay, it seems reasonable enough to have the `[[clang::lto_visibility_public]]` 
attribute override the `--lto-whole-program-visibility` flag.

What I'm not sure about though is whether `__declspec(uuid)` and `std`/`stdext` 
in `/MT`/`/MTd` should also override it.

Fortunately we don't need to solve that now because those are Windows-specific 
attributes/features and we don't support passing  
`--lto-whole-program-visibility` to Windows linkers.

So LGTM if you only mention `[[clang::lto_visibility_public]]` in your change 
to the documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127876

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

[PATCH] D127876: [clang] Don't emit type test/assume for virtual classes that should never participate in WPD

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D127876#3586154 , @aeubanks wrote:

> In D127876#3586134 , @pcc wrote:
>
>> This diverges from the documented behavior of 
>> `-lto-whole-program-visibility`. The flag is meant to give all classes 
>> hidden LTO visibility, but now it only does that to some of them.
>
> perhaps I'm misunderstanding, but even with `-lto-whole-program-visibility` 
> classes explicitly marked with public LTO visibility still shouldn't 
> participate in WPD?

We documented the feature in D75655  and there 
it says "all classes" (and still does).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127876

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

[PATCH] D127876: [clang] Don't emit type test/assume for virtual classes that should never participate in WPD

[Peter Collingbourne via Phabricator via cfe-commits]

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

This diverges from the documented behavior of `-lto-whole-program-visibility`. 
The flag is meant to give all classes hidden LTO visibility, but now it only 
does that to some of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127876

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

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: llvm/tools/gold/gold-plugin.cpp:44-52
 // FIXME: remove this declaration when we stop maintaining Ubuntu Quantal and
 // Precise and Debian Wheezy (binutils 2.23 is required)
 #define LDPO_PIE 3
 
 #define LDPT_GET_SYMBOLS_V3 28
 
 // FIXME: Remove when binutils 2.31 (containing gold 1.16) is the minimum

Can we remove this stuff now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D120727#3512177 , @paulkirth wrote:

> Hi, we're seeing some failures in Fuchsia's Clang CI. Our runtimes build 
> seems to be unable to find `cxxabi.h`.
>
> The failing bot can be found here: 
> https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814278370664903633/overview

Looks like we're seeing the same issue on the sanitizer CI: 
https://lab.llvm.org/buildbot/#/builders/37/builds/13232


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120727

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

[PATCH] D115844: [ubsan] Using metadata instead of prologue data for function sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1843
+
+  auto *FTRTTIProxy = new llvm::GlobalVariable(
+  TheModule, Addr->getType(),

ychen wrote:
> pcc wrote:
> > Are these proxy variables necessary? I think that now that we have custom 
> > code generation for this you should be able to use a GOTPCREL relocation to 
> > refer to the global.
> I attempted the GOTPCREL approach in a local branch. It didn't work for a 
> reason that I couldn't remember off the top of my head. I'll find out.
> 
> > I think that now that we have custom code generation for this 
> 
> Sorry, I don't quite follow which `custom code generation` you are referring 
> to. Do you mean the changes in `AsmPrinter.cpp`?
> Sorry, I don't quite follow which custom code generation you are referring 
> to. Do you mean the changes in AsmPrinter.cpp?

Yes, that's what I meant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115844

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

[PATCH] D115844: [ubsan] Using metadata instead of prologue data for function sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1843
+
+  auto *FTRTTIProxy = new llvm::GlobalVariable(
+  TheModule, Addr->getType(),

Are these proxy variables necessary? I think that now that we have custom code 
generation for this you should be able to use a GOTPCREL relocation to refer to 
the global.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115844

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

[PATCH] D120862: Sema: Allow scoped enums as source type for integral conversion.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120862

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

[PATCH] D119296: KCFI sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D119296#3467905 , @samitolvanen 
wrote:

> In D119296#3466027 , @joaomoreira 
> wrote:
>
>> I played a little bit with kcfi and here are some thoughts:
>>
>> - under -Os I saw functions being inlined, regardless of the source code 
>> calling them indirectly. In these scenarios, the KCFI check was still in 
>> place, even though there was not a pointer involved in the call. Although 
>> not semantically incorrect, it would be great to prevent the unnecessary 
>> overhead (see attached source/compile it with -Os and -fsanitize=kcfi).
>
> Yes, I suspect this might be an issue with Clang's existing CFI schemes too, 
> and would probably require an additional pass to drop checks before calls 
> that were either inlined or optimized into direct calls.

IIRC, this wasn't really an issue for -fsanitize=cfi because 99% of the time 
indirect calls that were going to resolve to direct calls had already done so 
by the time we got to the LowerTypeTests pass (which was only run during LTO).

It would probably be enough to have a pass at the end of the target-independent 
pipeline like you're doing here. You can probably check how effective it is by 
experimentally adding an assertion or something to the backend code so it 
complains if a direct call is found.

I probably wouldn't call the pass "KCFI" though as that implies that the pass 
itself implements KCFI. Maybe something like KCFIRemoveChecks would be better. 
Or maybe this is simple enough to add to another pass like InstCombine instead 
of adding a new one.

(That's if you don't go for the operand bundle approach described below.)

>> - I noticed that KCFI checks are placed much before the indirect call 
>> arguments are properly placed in the passing registers. This makes the 
>> position of the check unpredictable. I assume this is a bad thing in case 
>> the kernel eventually decides to come up with an approach that uses 
>> alternatives CFI schemes through text-patching.
>
> It shouldn't matter for text patching as we'll still know the exact location 
> of the instruction sequence that we want to patch by looking at the 
> `.kcfi_traps` section.
>
>> Because of things like the above, in the past I decided to implement these 
>> things in the very backend of the compiler, so other optimizations would not 
>> break the layout nor leave dummy checks around. I find it nice to have this 
>> implemented as a more architecture general feature, but maybe it would be 
>> cool to have a finalization pass in the X86 backend just to tie things.
>
> @pcc, any thoughts about these issues?

This seems like a reasonable approach, and was also the approach taken for the 
PAuth ABI. The PAuth ABI attaches an operand bundle to the call instruction and 
arranges for the code for the check to be generated together with the call. 
This helps with avoiding spills of the verified function pointer between the 
check and the call. The code isn't upstream but is available on this branch: 
https://github.com/pcc/llvm-project/tree/apple-pac4

Grep for something like `undle.*ptrauth` and you should find the relevant code.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2287
+const Twine  = ".weak __kcfi_typeid_" + Name + "\n" +
+   ".set __kcfi_typeid_" + Name + ", " +
+   Twine(Id->getZExtValue()) + "\n";

Won't this bloat symbol tables with mostly unused entries? I was thinking you 
could make them hidden, but that wouldn't have an effect on kernel modules. 
Maybe you should have this be opt-in with an attribute and have the kernel's 
asmlinkage expand to the attribute.



Comment at: llvm/lib/Transforms/IPO/KCFI.cpp:62
+
+auto *Target = dyn_cast_or_null(CI->getArgOperand(0));
+if (!Target || !Target->hasFnAttribute("kcfi") || !Target->hasPrefixData())

Should it look through bitcasts? (Probably moot with the imminent switch to 
opaque pointers though.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296

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

[PATCH] D119296: KCFI sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

> Note that if additional data has been injected between the KCFI
> type identifier and the start of the function, e.g. by using
> -fpatchable-function-entry, the offset in bytes must be specified
> using -fsanitize-kcfi-offset= to avoid errors. The offset
> must be the same for all indirectly called functions in every
> translation unit.

On x86 the specific constant 6 is necessary to ensure that the constant 
embedded in the cmpl operand can't be used as a gadget. So any value other than 
6 will potentially impact the security of KCFI.

I would prefer not to design an interaction between -fpatchable-function-entry 
and KCFI until the specific use case is known.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296

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

[PATCH] D85649: [AArch64] PAC/BTI code generation for LLVM generated functions

[Peter Collingbourne via Phabricator via cfe-commits]

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

In D85649#2281439 , @chill wrote:

> It looks like the only value that makes sense is `Error` - any other policy 
> (existing or not) would potentially lead to meaningfully different code 
> generated with or without LTO.

It should have been (the currently non-existent) "Min" -- with "Error" we get 
different behavior at link time with and without LTO, as a mixture of BTI and 
non-BTI objects ought to disable BTI in the final linked object, instead of 
producing an error. The code generation should be controlled by the function 
attribute so you ought to get the same code generated with and without LTO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85649

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

[PATCH] D120862: Sema: Allow scoped enums as source type for integral conversion.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added a reviewer: rsmith.
Herald added a project: All.
pcc requested review of this revision.
Herald added a project: clang.

Fixes pr54158.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120862

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/cxx17-enum-scoped.cpp


Index: clang/test/SemaCXX/cxx17-enum-scoped.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx17-enum-scoped.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -pedantic -std=c++17 -verify -triple 
x86_64-apple-darwin %s
+
+// expected-no-diagnostics
+
+namespace PR54158 {
+  enum class A : int;
+  enum class B : int;
+  B x{A{}};
+}
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -420,7 +420,7 @@
   //value when converted back to the original type.
   case ICK_Integral_Conversion:
   IntegralConversion: {
-assert(FromType->isIntegralOrUnscopedEnumerationType());
+assert(FromType->isIntegralOrEnumerationType());
 assert(ToType->isIntegralOrUnscopedEnumerationType());
 const bool FromSigned = FromType->isSignedIntegerOrEnumerationType();
 const unsigned FromWidth = Ctx.getIntWidth(FromType);


Index: clang/test/SemaCXX/cxx17-enum-scoped.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx17-enum-scoped.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -pedantic -std=c++17 -verify -triple x86_64-apple-darwin %s
+
+// expected-no-diagnostics
+
+namespace PR54158 {
+  enum class A : int;
+  enum class B : int;
+  B x{A{}};
+}
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -420,7 +420,7 @@
   //value when converted back to the original type.
   case ICK_Integral_Conversion:
   IntegralConversion: {
-assert(FromType->isIntegralOrUnscopedEnumerationType());
+assert(FromType->isIntegralOrEnumerationType());
 assert(ToType->isIntegralOrUnscopedEnumerationType());
 const bool FromSigned = FromType->isSignedIntegerOrEnumerationType();
 const unsigned FromWidth = Ctx.getIntWidth(FromType);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

[Peter Collingbourne 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 rG82e5f951fd6e: AST: Move __va_list tag back to std 
conditionally on AArch64. (authored by pcc).

Changed prior to commit:
  https://reviews.llvm.org/D116774?vs=409716=409734#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/arm64-be-hfa-vararg.c
  clang/test/Headers/stdarg.cpp

Index: clang/test/Headers/stdarg.cpp
===
--- clang/test/Headers/stdarg.cpp
+++ clang/test/Headers/stdarg.cpp
@@ -15,7 +15,7 @@
 
 #include 
 
-// AARCH64-C: define {{.*}} @f(i32 noundef %n, %"struct.std::__va_list"* noundef %list)
+// AARCH64-C: define {{.*}} @f(i32 noundef %n, %struct.__va_list* noundef %list)
 // AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 noundef %n, %"struct.std::__va_list"* noundef %list)
 // X86_64-C: define {{.*}} @f(i32 noundef %n, %struct.__va_list_tag* noundef %list)
 // X86_64-CXX: define {{.*}} @_Z1fiP13__va_list_tag(i32 noundef %n, %struct.__va_list_tag* noundef %list)
Index: clang/test/CodeGen/arm64-be-hfa-vararg.c
===
--- clang/test/CodeGen/arm64-be-hfa-vararg.c
+++ clang/test/CodeGen/arm64-be-hfa-vararg.c
@@ -4,12 +4,12 @@
 
 // A single member HFA must be aligned just like a non-HFA register argument.
 double callee(int a, ...) {
-// CHECK: [[REGPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 2
+// CHECK: [[REGPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 2
 // CHECK: [[REGP:%.*]] = load i8*, i8** [[REGPP]], align 8
 // CHECK: [[OFFSET0:%.*]] = getelementptr inbounds i8, i8* [[REGP]], i32 {{.*}}
 // CHECK: [[OFFSET1:%.*]] = getelementptr inbounds i8, i8* [[OFFSET0]], i64 8
 
-// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 0
+// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 0
 // CHECK: [[MEMP:%.*]] = load i8*, i8** [[MEMPP]], align 8
 // CHECK: [[NEXTP:%.*]] = getelementptr inbounds i8, i8* [[MEMP]], i64 8
 // CHECK: store i8* [[NEXTP]], i8** [[MEMPP]], align 8
Index: clang/test/CodeGen/aarch64-varargs.c
===
--- clang/test/CodeGen/aarch64-varargs.c
+++ clang/test/CodeGen/aarch64-varargs.c
@@ -11,18 +11,18 @@
 int simple_int(void) {
 // CHECK-LABEL: define{{.*}} i32 @simple_int
   return va_arg(the_list, int);
-// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[EARLY_ONSTACK:%[a-z_0-9]+]] = icmp sge i32 [[GR_OFFS]], 0
 // CHECK: br i1 [[EARLY_ONSTACK]], label %[[VAARG_ON_STACK:[a-z_.0-9]+]], label %[[VAARG_MAYBE_REG:[a-z_.0-9]+]]
 
 // CHECK: [[VAARG_MAYBE_REG]]
 // CHECK: [[NEW_REG_OFFS:%[a-z_0-9]+]] = add i32 [[GR_OFFS]], 8
-// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[INREG:%[a-z_0-9]+]] = icmp sle i32 [[NEW_REG_OFFS]], 0
 // CHECK: br i1 [[INREG]], label %[[VAARG_IN_REG:[a-z_.0-9]+]], label %[[VAARG_ON_STACK]]
 
 // CHECK: [[VAARG_IN_REG]]
-// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1)
+// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 1)
 // CHECK: [[REG_ADDR:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[REG_TOP]], i32 [[GR_OFFS]]
 // CHECK-BE: [[REG_ADDR_ALIGNED:%[0-9]+]] = getelementptr inbounds i8, i8* [[REG_ADDR]], i64 4
 // CHECK-BE: [[FROMREG_ADDR:%[a-z_0-9]+]] = bitcast i8* [[REG_ADDR_ALIGNED]] to i32*
@@ -30,9 +30,9 @@
 // CHECK: br label %[[VAARG_END:[a-z._0-9]+]]
 
 // CHECK: [[VAARG_ON_STACK]]
-// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 0)
+// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 0)
 // CHECK: [[NEW_STACK:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[STACK]], i64 8
-// CHECK: store i8* [[NEW_STACK]], i8** 

[PATCH] D116773: AST: Make getEffectiveDeclContext() a member function of ItaniumMangleContextImpl. NFCI.

[Peter Collingbourne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG18ead23385a4: AST: Make getEffectiveDeclContext() a member 
function of… (authored by pcc).

Changed prior to commit:
  https://reviews.llvm.org/D116773?vs=398007=409733#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116773

Files:
  clang/lib/AST/ItaniumMangle.cpp

Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -40,65 +40,10 @@
 
 namespace {
 
-/// Retrieve the declaration context that should be used when mangling the given
-/// declaration.
-static const DeclContext *getEffectiveDeclContext(const Decl *D) {
-  // The ABI assumes that lambda closure types that occur within
-  // default arguments live in the context of the function. However, due to
-  // the way in which Clang parses and creates function declarations, this is
-  // not the case: the lambda closure type ends up living in the context
-  // where the function itself resides, because the function declaration itself
-  // had not yet been created. Fix the context here.
-  if (const CXXRecordDecl *RD = dyn_cast(D)) {
-if (RD->isLambda())
-  if (ParmVarDecl *ContextParam
-= dyn_cast_or_null(RD->getLambdaContextDecl()))
-return ContextParam->getDeclContext();
-  }
-
-  // Perform the same check for block literals.
-  if (const BlockDecl *BD = dyn_cast(D)) {
-if (ParmVarDecl *ContextParam
-  = dyn_cast_or_null(BD->getBlockManglingContextDecl()))
-  return ContextParam->getDeclContext();
-  }
-
-  const DeclContext *DC = D->getDeclContext();
-  if (isa(DC) || isa(DC) ||
-  isa(DC)) {
-return getEffectiveDeclContext(cast(DC));
-  }
-
-  if (const auto *VD = dyn_cast(D))
-if (VD->isExternC())
-  return VD->getASTContext().getTranslationUnitDecl();
-
-  if (const auto *FD = dyn_cast(D))
-if (FD->isExternC())
-  return FD->getASTContext().getTranslationUnitDecl();
-
-  return DC->getRedeclContext();
-}
-
-static const DeclContext *getEffectiveParentContext(const DeclContext *DC) {
-  return getEffectiveDeclContext(cast(DC));
-}
-
 static bool isLocalContainerContext(const DeclContext *DC) {
   return isa(DC) || isa(DC) || isa(DC);
 }
 
-static const RecordDecl *GetLocalClassDecl(const Decl *D) {
-  const DeclContext *DC = getEffectiveDeclContext(D);
-  while (!DC->isNamespace() && !DC->isTranslationUnit()) {
-if (isLocalContainerContext(DC))
-  return dyn_cast(D);
-D = cast(DC);
-DC = getEffectiveDeclContext(D);
-  }
-  return nullptr;
-}
-
 static const FunctionDecl *getStructor(const FunctionDecl *fn) {
   if (const FunctionTemplateDecl *ftd = fn->getPrimaryTemplate())
 return ftd->getTemplatedDecl();
@@ -249,6 +194,14 @@
 return DiscriminatorOverride;
   }
 
+  const DeclContext *getEffectiveDeclContext(const Decl *D);
+  const DeclContext *getEffectiveParentContext(const DeclContext *DC) {
+return getEffectiveDeclContext(cast(DC));
+  }
+
+  bool isInternalLinkageDecl(const NamedDecl *ND);
+  const DeclContext *IgnoreLinkageSpecDecls(const DeclContext *DC);
+
   /// @}
 };
 
@@ -427,6 +380,15 @@
 
   ASTContext () const { return Context.getASTContext(); }
 
+  bool isStd(const NamespaceDecl *NS);
+  bool isStdNamespace(const DeclContext *DC);
+
+  const RecordDecl *GetLocalClassDecl(const Decl *D);
+  const DeclContext *IgnoreLinkageSpecDecls(const DeclContext *DC);
+  bool isSpecializedAs(QualType S, llvm::StringRef Name, QualType A);
+  bool isStdCharSpecialization(const ClassTemplateSpecializationDecl *SD,
+   llvm::StringRef Name, bool HasAllocator);
+
 public:
   CXXNameMangler(ItaniumMangleContextImpl , raw_ostream _,
  const NamedDecl *D = nullptr, bool NullOut_ = false)
@@ -628,7 +590,48 @@
 
 }
 
-static bool isInternalLinkageDecl(const NamedDecl *ND) {
+/// Retrieve the declaration context that should be used when mangling the given
+/// declaration.
+const DeclContext *
+ItaniumMangleContextImpl::getEffectiveDeclContext(const Decl *D) {
+  // The ABI assumes that lambda closure types that occur within
+  // default arguments live in the context of the function. However, due to
+  // the way in which Clang parses and creates function declarations, this is
+  // not the case: the lambda closure type ends up living in the context
+  // where the function itself resides, because the function declaration itself
+  // had not yet been created. Fix the context here.
+  if (const CXXRecordDecl *RD = dyn_cast(D)) {
+if (RD->isLambda())
+  if (ParmVarDecl *ContextParam =
+  dyn_cast_or_null(RD->getLambdaContextDecl()))
+return ContextParam->getDeclContext();
+  }
+
+  // Perform the same check for block literals.
+  if (const BlockDecl *BD = dyn_cast(D)) {
+if 

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc marked 2 inline comments as done.
pcc added a comment.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc updated this revision to Diff 409716.
pcc added a comment.

Use isARM() etc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/arm64-be-hfa-vararg.c
  clang/test/Headers/stdarg.cpp

Index: clang/test/Headers/stdarg.cpp
===
--- clang/test/Headers/stdarg.cpp
+++ clang/test/Headers/stdarg.cpp
@@ -15,7 +15,7 @@
 
 #include 
 
-// AARCH64-C: define {{.*}} @f(i32 noundef %n, %"struct.std::__va_list"* noundef %list)
+// AARCH64-C: define {{.*}} @f(i32 noundef %n, %struct.__va_list* noundef %list)
 // AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 noundef %n, %"struct.std::__va_list"* noundef %list)
 // X86_64-C: define {{.*}} @f(i32 noundef %n, %struct.__va_list_tag* noundef %list)
 // X86_64-CXX: define {{.*}} @_Z1fiP13__va_list_tag(i32 noundef %n, %struct.__va_list_tag* noundef %list)
Index: clang/test/CodeGen/arm64-be-hfa-vararg.c
===
--- clang/test/CodeGen/arm64-be-hfa-vararg.c
+++ clang/test/CodeGen/arm64-be-hfa-vararg.c
@@ -4,12 +4,12 @@
 
 // A single member HFA must be aligned just like a non-HFA register argument.
 double callee(int a, ...) {
-// CHECK: [[REGPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 2
+// CHECK: [[REGPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 2
 // CHECK: [[REGP:%.*]] = load i8*, i8** [[REGPP]], align 8
 // CHECK: [[OFFSET0:%.*]] = getelementptr inbounds i8, i8* [[REGP]], i32 {{.*}}
 // CHECK: [[OFFSET1:%.*]] = getelementptr inbounds i8, i8* [[OFFSET0]], i64 8
 
-// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 0
+// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 0
 // CHECK: [[MEMP:%.*]] = load i8*, i8** [[MEMPP]], align 8
 // CHECK: [[NEXTP:%.*]] = getelementptr inbounds i8, i8* [[MEMP]], i64 8
 // CHECK: store i8* [[NEXTP]], i8** [[MEMPP]], align 8
Index: clang/test/CodeGen/aarch64-varargs.c
===
--- clang/test/CodeGen/aarch64-varargs.c
+++ clang/test/CodeGen/aarch64-varargs.c
@@ -11,18 +11,18 @@
 int simple_int(void) {
 // CHECK-LABEL: define{{.*}} i32 @simple_int
   return va_arg(the_list, int);
-// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[EARLY_ONSTACK:%[a-z_0-9]+]] = icmp sge i32 [[GR_OFFS]], 0
 // CHECK: br i1 [[EARLY_ONSTACK]], label %[[VAARG_ON_STACK:[a-z_.0-9]+]], label %[[VAARG_MAYBE_REG:[a-z_.0-9]+]]
 
 // CHECK: [[VAARG_MAYBE_REG]]
 // CHECK: [[NEW_REG_OFFS:%[a-z_0-9]+]] = add i32 [[GR_OFFS]], 8
-// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[INREG:%[a-z_0-9]+]] = icmp sle i32 [[NEW_REG_OFFS]], 0
 // CHECK: br i1 [[INREG]], label %[[VAARG_IN_REG:[a-z_.0-9]+]], label %[[VAARG_ON_STACK]]
 
 // CHECK: [[VAARG_IN_REG]]
-// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1)
+// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 1)
 // CHECK: [[REG_ADDR:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[REG_TOP]], i32 [[GR_OFFS]]
 // CHECK-BE: [[REG_ADDR_ALIGNED:%[0-9]+]] = getelementptr inbounds i8, i8* [[REG_ADDR]], i64 4
 // CHECK-BE: [[FROMREG_ADDR:%[a-z_0-9]+]] = bitcast i8* [[REG_ADDR_ALIGNED]] to i32*
@@ -30,9 +30,9 @@
 // CHECK: br label %[[VAARG_END:[a-z._0-9]+]]
 
 // CHECK: [[VAARG_ON_STACK]]
-// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 0)
+// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 0)
 // CHECK: [[NEW_STACK:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[STACK]], i64 8
-// CHECK: store i8* [[NEW_STACK]], i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 0)
+// CHECK: store i8* [[NEW_STACK]], i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 0)
 // CHECK-BE: 

[PATCH] D115844: [ubsan] Using metadata instead of prologue data for function sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D115844#3321235 , @ychen wrote:

> In D115844#3321190 , @pcc wrote:
>
>> On the bug you have:
>>
>>   define internal fastcc void 
>> @_Z4callIiE4taskv.resume(%_Z4callIiE4taskv.Frame* noalias nonnull 
>> align 8 dereferenceable(24
>>   ) %FramePtr) #1 prologue <{ i32, i32 }> <{ i32 846595819, i32 
>> trunc (i64 sub (i64 ptrtoint (i8** @1 to i64), i64 ptrtoint (void ()* 
>> @_Z4callIiE4taskv to i64)) to i32) }> {...}
>>
>> Is it possible for the C/C++ code to take the address of the function 
>> `_Z4callIiE4taskv.resume` and call it indirectly?
>
> `*.resume` is a compiler inserted function that is opaque to the programmer. 
> It is called indirectly most of the time if not all the time.

Yes, but not indirectly called from C/C++ but rather from compiler-generated 
code right? That's presumably why this patch + D116130 
 worked for @zhuhan0.

>> If not, it seems like the right fix would be to arrange for the prologue 
>> data to be dropped on the `.resume` function instead of duplicating it 
>> there. I would also imagine that whatever signature you have on the 
>> `.resume` function would be incorrect since it appears that the coro 
>> splitting pass will use a different function signature for that function.
>
> That is addressed by D116130 . @rjmccall 
> suggested the direction of this patch (which I agreed) 
> https://reviews.llvm.org/D114728#3159303.

Okay. It seems unfortunate to have to special-case this just because it uses 
relative relocations. But that's probably the best that we can do without 
changing the global variable initializer representation (as well as 
prefix/prologue data) to be blob plus relocations.




Comment at: clang/lib/CodeGen/CGExpr.cpp:5171
   (!TargetDecl || !isa(TargetDecl))) {
+assert((CGM.getCodeGenOpts().CodeModel == "default" ||
+CGM.getCodeGenOpts().CodeModel == "small") &&

What happens when building with other code models? Hopefully we get an error of 
some sort before hitting this assertion failure, right?



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:843
+
+  // Emit the function prologue data for the indirect call sanitizer.
+  if (const MDNode *MD = F.getMetadata(LLVMContext::MD_func_sanitize)) {

What if we have both prologue data and this metadata? Should it be an error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115844

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

[PATCH] D115844: [ubsan] Using metadata instead of prologue data for function sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

On the bug you have:

  define internal fastcc void 
@_Z4callIiE4taskv.resume(%_Z4callIiE4taskv.Frame* noalias nonnull align 
8 dereferenceable(24
  ) %FramePtr) #1 prologue <{ i32, i32 }> <{ i32 846595819, i32 
trunc (i64 sub (i64 ptrtoint (i8** @1 to i64), i64 ptrtoint (void ()* 
@_Z4callIiE4taskv to i64)) to i32) }> {...}

Is it possible for the C/C++ code to take the address of the function 
`_Z4callIiE4taskv.resume` and call it indirectly? If not, it seems like the 
right fix would be to arrange for the prologue data to be dropped on the 
`.resume` function instead of duplicating it there. I would also imagine that 
whatever signature you have on the `.resume` function would be incorrect since 
it appears that the coro splitting pass will use a different function signature 
for that function.

Note that D119296  will have the same problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115844

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

[PATCH] D119367: [HWASan] Allow no_sanitize(..) and change metadata passing.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In addition to the .bc support we will also need support for reading/writing 
.ll files.




Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1721
-static DenseSet getExcludedGlobals(Module ) {
-  NamedMDNode *Globals = M.getNamedMetadata("llvm.asan.globals");
-  if (!Globals)

Looks like you'll need to update 
llvm/test/Instrumentation/HWAddressSanitizer/globals.ll to use the new 
attribute instead (after adding the .ll support).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119367

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

[PATCH] D119296: KCFI sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3168
+  -1);
+  llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash);
+  llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont");

pcc wrote:
> samitolvanen wrote:
> > pcc wrote:
> > > We considered a scheme like this before and one problem that we 
> > > discovered with comparing the hash in this way is that it can produce 
> > > gadgets, e.g.
> > > ```
> > > movabs $0x0123456789abcdef, %rax
> > > cmp %rax, ...
> > > ```
> > > the `cmp`instruction ends up being a valid target address because the 
> > > `movabs` instruction ends in the hash. The way we thought about solving 
> > > this was to introduce a new intrinsic that would materialize the constant 
> > > without these gadgets (e.g. invert the `movabs` operand and follow it by 
> > > a `not`).
> > Yes, that's a concern with this approach, at least on x86_64. As the hash 
> > is more or less random, I assume you'd have to actually check that the 
> > inverted form won't have useful gadgets either, and potentially split the 
> > single `movabs` into multiple instructions if needed etc. Did you ever 
> > start work on the intrinsic or was that just an idea?
> The likelihood of the inverted operand having gadgets seems equal to that of 
> any other piece of code having gadgets (here I'm just talking about KCFI 
> gadgets, not any other kind of gadget). And if you're using a fixed 2-byte 
> prefix it would be impossible for the `movabs` operand to itself be a gadget. 
> So I don't think it would be necessary to check the inverted operand 
> specifically for gadgets.
> 
> You might want to consider selecting the fixed prefix more carefully. It may 
> be worth looking for a prefix that is less likely to appear in generated code 
> (e.g. by taking a histogram of 2-byte sequences in a corpus of libraries) 
> rather than choosing one arbitrarily.
Also the intrinsic was just an idea, we never implemented it because we ended 
up going with the currently implemented strategy for the CFI sanitizers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296

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

[PATCH] D119296: KCFI sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3168
+  -1);
+  llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash);
+  llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont");

samitolvanen wrote:
> pcc wrote:
> > We considered a scheme like this before and one problem that we discovered 
> > with comparing the hash in this way is that it can produce gadgets, e.g.
> > ```
> > movabs $0x0123456789abcdef, %rax
> > cmp %rax, ...
> > ```
> > the `cmp`instruction ends up being a valid target address because the 
> > `movabs` instruction ends in the hash. The way we thought about solving 
> > this was to introduce a new intrinsic that would materialize the constant 
> > without these gadgets (e.g. invert the `movabs` operand and follow it by a 
> > `not`).
> Yes, that's a concern with this approach, at least on x86_64. As the hash is 
> more or less random, I assume you'd have to actually check that the inverted 
> form won't have useful gadgets either, and potentially split the single 
> `movabs` into multiple instructions if needed etc. Did you ever start work on 
> the intrinsic or was that just an idea?
The likelihood of the inverted operand having gadgets seems equal to that of 
any other piece of code having gadgets (here I'm just talking about KCFI 
gadgets, not any other kind of gadget). And if you're using a fixed 2-byte 
prefix it would be impossible for the `movabs` operand to itself be a gadget. 
So I don't think it would be necessary to check the inverted operand 
specifically for gadgets.

You might want to consider selecting the fixed prefix more carefully. It may be 
worth looking for a prefix that is less likely to appear in generated code 
(e.g. by taking a histogram of 2-byte sequences in a corpus of libraries) 
rather than choosing one arbitrarily.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296

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

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc updated this revision to Diff 407227.
pcc retitled this revision from "AST: Move __va_list tag to the top level on 
ARM architectures." to "AST: Move __va_list tag back to std conditionally on 
AArch64.".
pcc edited the summary of this revision.
pcc added a comment.

Make it conditional


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/arm64-be-hfa-vararg.c
  clang/test/Headers/stdarg.cpp

Index: clang/test/Headers/stdarg.cpp
===
--- clang/test/Headers/stdarg.cpp
+++ clang/test/Headers/stdarg.cpp
@@ -15,7 +15,7 @@
 
 #include 
 
-// AARCH64-C: define {{.*}} @f(i32 noundef %n, %"struct.std::__va_list"* noundef %list)
+// AARCH64-C: define {{.*}} @f(i32 noundef %n, %struct.__va_list* noundef %list)
 // AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 noundef %n, %"struct.std::__va_list"* noundef %list)
 // X86_64-C: define {{.*}} @f(i32 noundef %n, %struct.__va_list_tag* noundef %list)
 // X86_64-CXX: define {{.*}} @_Z1fiP13__va_list_tag(i32 noundef %n, %struct.__va_list_tag* noundef %list)
Index: clang/test/CodeGen/arm64-be-hfa-vararg.c
===
--- clang/test/CodeGen/arm64-be-hfa-vararg.c
+++ clang/test/CodeGen/arm64-be-hfa-vararg.c
@@ -4,12 +4,12 @@
 
 // A single member HFA must be aligned just like a non-HFA register argument.
 double callee(int a, ...) {
-// CHECK: [[REGPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 2
+// CHECK: [[REGPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 2
 // CHECK: [[REGP:%.*]] = load i8*, i8** [[REGPP]], align 8
 // CHECK: [[OFFSET0:%.*]] = getelementptr inbounds i8, i8* [[REGP]], i32 {{.*}}
 // CHECK: [[OFFSET1:%.*]] = getelementptr inbounds i8, i8* [[OFFSET0]], i64 8
 
-// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 0
+// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 0
 // CHECK: [[MEMP:%.*]] = load i8*, i8** [[MEMPP]], align 8
 // CHECK: [[NEXTP:%.*]] = getelementptr inbounds i8, i8* [[MEMP]], i64 8
 // CHECK: store i8* [[NEXTP]], i8** [[MEMPP]], align 8
Index: clang/test/CodeGen/aarch64-varargs.c
===
--- clang/test/CodeGen/aarch64-varargs.c
+++ clang/test/CodeGen/aarch64-varargs.c
@@ -11,18 +11,18 @@
 int simple_int(void) {
 // CHECK-LABEL: define{{.*}} i32 @simple_int
   return va_arg(the_list, int);
-// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[EARLY_ONSTACK:%[a-z_0-9]+]] = icmp sge i32 [[GR_OFFS]], 0
 // CHECK: br i1 [[EARLY_ONSTACK]], label %[[VAARG_ON_STACK:[a-z_.0-9]+]], label %[[VAARG_MAYBE_REG:[a-z_.0-9]+]]
 
 // CHECK: [[VAARG_MAYBE_REG]]
 // CHECK: [[NEW_REG_OFFS:%[a-z_0-9]+]] = add i32 [[GR_OFFS]], 8
-// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[INREG:%[a-z_0-9]+]] = icmp sle i32 [[NEW_REG_OFFS]], 0
 // CHECK: br i1 [[INREG]], label %[[VAARG_IN_REG:[a-z_.0-9]+]], label %[[VAARG_ON_STACK]]
 
 // CHECK: [[VAARG_IN_REG]]
-// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1)
+// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 1)
 // CHECK: [[REG_ADDR:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[REG_TOP]], i32 [[GR_OFFS]]
 // CHECK-BE: [[REG_ADDR_ALIGNED:%[0-9]+]] = getelementptr inbounds i8, i8* [[REG_ADDR]], i64 4
 // CHECK-BE: [[FROMREG_ADDR:%[a-z_0-9]+]] = bitcast i8* [[REG_ADDR_ALIGNED]] to i32*
@@ -30,9 +30,9 @@
 // CHECK: br label %[[VAARG_END:[a-z._0-9]+]]
 
 // CHECK: [[VAARG_ON_STACK]]
-// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 0)
+// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 0)
 // CHECK: [[NEW_STACK:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[STACK]], i64 8
-// CHECK: store i8* [[NEW_STACK]], i8** getelementptr inbounds 

[PATCH] D119296: KCFI sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3168
+  -1);
+  llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash);
+  llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont");

We considered a scheme like this before and one problem that we discovered with 
comparing the hash in this way is that it can produce gadgets, e.g.
```
movabs $0x0123456789abcdef, %rax
cmp %rax, ...
```
the `cmp`instruction ends up being a valid target address because the `movabs` 
instruction ends in the hash. The way we thought about solving this was to 
introduce a new intrinsic that would materialize the constant without these 
gadgets (e.g. invert the `movabs` operand and follow it by a `not`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296

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

[PATCH] D118171: [HWASan] Add __hwasan_init to .preinit_array.

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118171

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

[PATCH] D118171: [HWASan] Add __hwasan_init to .preinit_array.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:842
+  if (!Args.hasArg(options::OPT_shared))
+SharedRuntimes.push_back("hwasan-preinit");
 }

morehouse wrote:
> eugenis wrote:
> > pcc wrote:
> > > Shouldn't it be added to `HelperStaticRuntimes`? Otherwise I'm not sure 
> > > how this would work because the library would need to be enclosed in 
> > > `--whole-archive` in order to be included in the link.
> > Right, good point.
> Yes, good catch!
Thanks. We'd better test on Android again because I imagine that the previous 
patch wouldn't have had any effect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118171

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

[PATCH] D118171: [HWASan] Add __hwasan_init to .preinit_array.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

What guarantee do we have that the `__hwasan_init` .preinit_array entry will be 
placed before the user's .preinit_array entries?




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:842
+  if (!Args.hasArg(options::OPT_shared))
+SharedRuntimes.push_back("hwasan-preinit");
 }

Shouldn't it be added to `HelperStaticRuntimes`? Otherwise I'm not sure how 
this would work because the library would need to be enclosed in 
`--whole-archive` in order to be included in the link.



Comment at: compiler-rt/lib/hwasan/hwasan_preinit.cpp:22
+__attribute__((section(".preinit_array"), used))
+void (*__local_hwasan_preinit)(void) = __hwasan_init;
+#endif

Can it be static?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118171

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

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

I'm not aware of any of those places causing an actual problem though? The AST 
isn't a stable interface, and __builtin_dump_struct is for debugging purposes 
only. I would expect debug info consumers to be able to handle __va_list in the 
global namespace as this is the status quo for C.

So I'm somewhat inclined to do the simple thing here first, and then look at 
making things more conditional if a problem comes up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D104830#3225370 , @jrtc27 wrote:

> Ping?

Sorry for the delay, D116774  should fix this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104830

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

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: rsmith, eugenis, jrtc27.
Herald added a subscriber: kristof.beyls.
pcc requested review of this revision.
Herald added a project: clang.

In post-commit feedback on D104830  Jessica 
Clarke pointed out that
unconditionally adding __va_list to the std namespace caused namespace
debug info to be emitted in C, which is not only inappropriate but
turned out to confuse the dtrace tool. Therefore, move __va_list to the
top level unconditionally so that the correct debug info is generated.

To avoid breaking name mangling for __va_list, teach the Itanium name
mangler to mangle it as if it were in the std namespace when targeting
ARM architectures. This logic is not needed for the Microsoft name
mangler because Microsoft platforms define va_list as a typedef of
char *.

It was also noted that 32-bit ARM has the same issue as was fixed
for 64-bit ARM in D104830 , so do the same 
for that architecture.

Depends on D116773 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116774

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/arm64-be-hfa-vararg.c
  clang/test/Headers/stdarg.cpp

Index: clang/test/Headers/stdarg.cpp
===
--- clang/test/Headers/stdarg.cpp
+++ clang/test/Headers/stdarg.cpp
@@ -15,8 +15,8 @@
 
 #include 
 
-// AARCH64-C: define {{.*}} @f(i32 %n, %"struct.std::__va_list"* %list)
-// AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 %n, %"struct.std::__va_list"* %list)
+// AARCH64-C: define {{.*}} @f(i32 %n, %struct.__va_list* %list)
+// AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 %n, %struct.__va_list* %list)
 // X86_64-C: define {{.*}} @f(i32 %n, %struct.__va_list_tag* %list)
 // X86_64-CXX: define {{.*}} @_Z1fiP13__va_list_tag(i32 %n, %struct.__va_list_tag* %list)
 // PPC64-C: define {{.*}} @f(i32 signext %n, i8* %list)
Index: clang/test/CodeGen/arm64-be-hfa-vararg.c
===
--- clang/test/CodeGen/arm64-be-hfa-vararg.c
+++ clang/test/CodeGen/arm64-be-hfa-vararg.c
@@ -4,12 +4,12 @@
 
 // A single member HFA must be aligned just like a non-HFA register argument.
 double callee(int a, ...) {
-// CHECK: [[REGPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 2
+// CHECK: [[REGPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 2
 // CHECK: [[REGP:%.*]] = load i8*, i8** [[REGPP]], align 8
 // CHECK: [[OFFSET0:%.*]] = getelementptr inbounds i8, i8* [[REGP]], i32 {{.*}}
 // CHECK: [[OFFSET1:%.*]] = getelementptr inbounds i8, i8* [[OFFSET0]], i64 8
 
-// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 0
+// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 0
 // CHECK: [[MEMP:%.*]] = load i8*, i8** [[MEMPP]], align 8
 // CHECK: [[NEXTP:%.*]] = getelementptr inbounds i8, i8* [[MEMP]], i64 8
 // CHECK: store i8* [[NEXTP]], i8** [[MEMPP]], align 8
Index: clang/test/CodeGen/aarch64-varargs.c
===
--- clang/test/CodeGen/aarch64-varargs.c
+++ clang/test/CodeGen/aarch64-varargs.c
@@ -11,18 +11,18 @@
 int simple_int(void) {
 // CHECK-LABEL: define{{.*}} i32 @simple_int
   return va_arg(the_list, int);
-// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[EARLY_ONSTACK:%[a-z_0-9]+]] = icmp sge i32 [[GR_OFFS]], 0
 // CHECK: br i1 [[EARLY_ONSTACK]], label %[[VAARG_ON_STACK:[a-z_.0-9]+]], label %[[VAARG_MAYBE_REG:[a-z_.0-9]+]]
 
 // CHECK: [[VAARG_MAYBE_REG]]
 // CHECK: [[NEW_REG_OFFS:%[a-z_0-9]+]] = add i32 [[GR_OFFS]], 8
-// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[INREG:%[a-z_0-9]+]] = icmp sle i32 [[NEW_REG_OFFS]], 0
 // CHECK: br i1 [[INREG]], label %[[VAARG_IN_REG:[a-z_.0-9]+]], label %[[VAARG_ON_STACK]]
 
 // CHECK: [[VAARG_IN_REG]]
-// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1)
+// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 1)
 // CHECK: [[REG_ADDR:%[a-z_0-9]+]] = 

[PATCH] D116773: AST: Make getEffectiveDeclContext() a member function of ItaniumMangleContextImpl. NFCI.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: eugenis, rsmith, jrtc27.
pcc requested review of this revision.
Herald added a project: clang.

In an upcoming change we are going to need to access mangler state
from the getEffectiveDeclContext() function. Therefore, make it a
member function of ItaniumMangleContextImpl. Any callers that are
not currently members of ItaniumMangleContextImpl or CXXNameMangler
are made members of one or the other depending on where they are
called from.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116773

Files:
  clang/lib/AST/ItaniumMangle.cpp

Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -40,65 +40,10 @@
 
 namespace {
 
-/// Retrieve the declaration context that should be used when mangling the given
-/// declaration.
-static const DeclContext *getEffectiveDeclContext(const Decl *D) {
-  // The ABI assumes that lambda closure types that occur within
-  // default arguments live in the context of the function. However, due to
-  // the way in which Clang parses and creates function declarations, this is
-  // not the case: the lambda closure type ends up living in the context
-  // where the function itself resides, because the function declaration itself
-  // had not yet been created. Fix the context here.
-  if (const CXXRecordDecl *RD = dyn_cast(D)) {
-if (RD->isLambda())
-  if (ParmVarDecl *ContextParam
-= dyn_cast_or_null(RD->getLambdaContextDecl()))
-return ContextParam->getDeclContext();
-  }
-
-  // Perform the same check for block literals.
-  if (const BlockDecl *BD = dyn_cast(D)) {
-if (ParmVarDecl *ContextParam
-  = dyn_cast_or_null(BD->getBlockManglingContextDecl()))
-  return ContextParam->getDeclContext();
-  }
-
-  const DeclContext *DC = D->getDeclContext();
-  if (isa(DC) || isa(DC) ||
-  isa(DC)) {
-return getEffectiveDeclContext(cast(DC));
-  }
-
-  if (const auto *VD = dyn_cast(D))
-if (VD->isExternC())
-  return VD->getASTContext().getTranslationUnitDecl();
-
-  if (const auto *FD = dyn_cast(D))
-if (FD->isExternC())
-  return FD->getASTContext().getTranslationUnitDecl();
-
-  return DC->getRedeclContext();
-}
-
-static const DeclContext *getEffectiveParentContext(const DeclContext *DC) {
-  return getEffectiveDeclContext(cast(DC));
-}
-
 static bool isLocalContainerContext(const DeclContext *DC) {
   return isa(DC) || isa(DC) || isa(DC);
 }
 
-static const RecordDecl *GetLocalClassDecl(const Decl *D) {
-  const DeclContext *DC = getEffectiveDeclContext(D);
-  while (!DC->isNamespace() && !DC->isTranslationUnit()) {
-if (isLocalContainerContext(DC))
-  return dyn_cast(D);
-D = cast(DC);
-DC = getEffectiveDeclContext(D);
-  }
-  return nullptr;
-}
-
 static const FunctionDecl *getStructor(const FunctionDecl *fn) {
   if (const FunctionTemplateDecl *ftd = fn->getPrimaryTemplate())
 return ftd->getTemplatedDecl();
@@ -249,6 +194,14 @@
 return DiscriminatorOverride;
   }
 
+  const DeclContext *getEffectiveDeclContext(const Decl *D);
+  const DeclContext *getEffectiveParentContext(const DeclContext *DC) {
+return getEffectiveDeclContext(cast(DC));
+  }
+
+  bool isInternalLinkageDecl(const NamedDecl *ND);
+  const DeclContext *IgnoreLinkageSpecDecls(const DeclContext *DC);
+
   /// @}
 };
 
@@ -427,6 +380,16 @@
 
   ASTContext () const { return Context.getASTContext(); }
 
+  bool isStd(const NamespaceDecl *NS);
+  bool isStdNamespace(const DeclContext *DC);
+
+  const RecordDecl *GetLocalClassDecl(const Decl *D);
+  const DeclContext *IgnoreLinkageSpecDecls(const DeclContext *DC);
+  bool isCharSpecialization(QualType T, const char *Name);
+  template 
+  bool isStreamCharSpecialization(const ClassTemplateSpecializationDecl *SD,
+  const char ()[StrLen]);
+
 public:
   CXXNameMangler(ItaniumMangleContextImpl , raw_ostream _,
  const NamedDecl *D = nullptr, bool NullOut_ = false)
@@ -628,7 +591,48 @@
 
 }
 
-static bool isInternalLinkageDecl(const NamedDecl *ND) {
+/// Retrieve the declaration context that should be used when mangling the given
+/// declaration.
+const DeclContext *
+ItaniumMangleContextImpl::getEffectiveDeclContext(const Decl *D) {
+  // The ABI assumes that lambda closure types that occur within
+  // default arguments live in the context of the function. However, due to
+  // the way in which Clang parses and creates function declarations, this is
+  // not the case: the lambda closure type ends up living in the context
+  // where the function itself resides, because the function declaration itself
+  // had not yet been created. Fix the context here.
+  if (const CXXRecordDecl *RD = dyn_cast(D)) {
+if (RD->isLambda())
+  if (ParmVarDecl *ContextParam =
+  dyn_cast_or_null(RD->getLambdaContextDecl()))
+

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108479

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

[PATCH] D115015: CodeGen: Strip exception specifications from function types in CFI type names.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added a reviewer: eugenis.
pcc requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

With C++17 the exception specification has been made part of the
function type, and therefore part of mangled type names.

However, it's valid to convert function pointers with an exception
specification to function pointers with the same argument and return
types but without an exception specification, which means that e.g. a
function of type "void () noexcept" can be called through a pointer
of type "void ()". We must therefore consider the two types to be
compatible for CFI purposes.

We can do this by stripping the exception specification before mangling
the type name, which is what this patch does.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115015

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGenCXX/cfi-icall-noexcept.cpp


Index: clang/test/CodeGenCXX/cfi-icall-noexcept.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cfi-icall-noexcept.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-icall 
-emit-llvm -std=c++17 -o - %s | FileCheck %s
+
+// Tests that exception specifiers are stripped when forming the
+// mangled CFI type name.
+
+void f() noexcept {}
+
+// CHECK: define{{.*}} void @_Z1fv({{.*}} !type [[TS1:![0-9]+]] !type 
[[TS2:![0-9]+]]
+
+// CHECK: [[TS1]] = !{i64 0, !"_ZTSFvvE"}
+// CHECK: [[TS2]] = !{i64 0, !"_ZTSFvvE.generalized"}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -6398,6 +6398,11 @@
 llvm::Metadata *
 CodeGenModule::CreateMetadataIdentifierImpl(QualType T, MetadataTypeMap ,
 StringRef Suffix) {
+  if (auto *FnType = T->getAs())
+T = getContext().getFunctionType(
+FnType->getReturnType(), FnType->getParamTypes(),
+FnType->getExtProtoInfo().withExceptionSpec(EST_None));
+
   llvm::Metadata * = Map[T.getCanonicalType()];
   if (InternalId)
 return InternalId;


Index: clang/test/CodeGenCXX/cfi-icall-noexcept.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cfi-icall-noexcept.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-icall -emit-llvm -std=c++17 -o - %s | FileCheck %s
+
+// Tests that exception specifiers are stripped when forming the
+// mangled CFI type name.
+
+void f() noexcept {}
+
+// CHECK: define{{.*}} void @_Z1fv({{.*}} !type [[TS1:![0-9]+]] !type [[TS2:![0-9]+]]
+
+// CHECK: [[TS1]] = !{i64 0, !"_ZTSFvvE"}
+// CHECK: [[TS2]] = !{i64 0, !"_ZTSFvvE.generalized"}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -6398,6 +6398,11 @@
 llvm::Metadata *
 CodeGenModule::CreateMetadataIdentifierImpl(QualType T, MetadataTypeMap ,
 StringRef Suffix) {
+  if (auto *FnType = T->getAs())
+T = getContext().getFunctionType(
+FnType->getReturnType(), FnType->getParamTypes(),
+FnType->getExtProtoInfo().withExceptionSpec(EST_None));
+
   llvm::Metadata * = Map[T.getCanonicalType()];
   if (InternalId)
 return InternalId;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Agreed that this should probably be done in the mangler, I'll try to take a 
look next week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104830

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

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D108479#3129577 , @rjmccall wrote:

> In D108479#3129571 , @jrtc27 wrote:
>
>> For CHERI there's the added complication that descriptors and trampolines 
>> can exist for security reasons when crossing security domains, and you 
>> absolutely should not let one compartment get pointers to the entry point of 
>> another compartment's function. You can hand it out if sealed or the 
>> permissions are cleared, as then you can't really do anything with it other 
>> than look at the integer address, but that seems a bit odd.
>
> That would be consistent with getting an unsigned pointer under pointer 
> authentication or an address with the THUMB bit potentially stripped: it's 
> just a raw address that you can't safely call.  In any case, I suspect it 
> would be fine for us to say that we just don't support this builtin when the 
> target is using weird function pointers unless we have some way to bypass the 
> special treatment in LLVM.
>
> I agree that "symbol" address is probably the wrong name.  Maybe 
> `__builtin_function_start` or something like that?  But before we go much 
> further on this, we should get confirmation from Peter that we're targeting 
> the right design.

Having this be defined to return the function body address (and diagnose at 
compile time if that's not supported) seem like the right design to me, and  
`__builtin_function_start` sounds like a good name.

Here's another example of a potential use case that came up in the course of 
the PAuth ABI work: testing whether a stack trace contains a particular 
function:
https://cs.android.com/android/platform/superproject/+/master:bionic/tests/execinfo_test.cpp;l=94

I think that no matter whether it's CFI, PAuth, CHERI or anything else, this 
test would want the address of the real function body. If the test is compiled 
on a platform where taking the real function body address is unsupported, it 
would fail at runtime if we just let this be compiled as a no-op, so we should 
diagnose the issue at compile time where possible.




Comment at: clang/test/SemaCXX/builtins.cpp:44
+void a(void) {}
+static_assert(__builtin_addressof_nocfi(a) == a, "");
+

I don't think this should always evaluate to true, should it? Maybe we should 
forbid these types of comparisons in integral constant expressions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108479

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

[PATCH] D113738: [LTO] Allow passing -Os/-Oz as the optimization level

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Please see D63976  where we rejected a similar 
change in favor of just letting this be controllable at compile time.

To the extent that the pass pipeline is affected by the size optimization 
level, I think we should change the passes so that they respect the 
`optsize`/`minsize` attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113738

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

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added subscribers: rjmccall, rsmith.
pcc added a comment.

(Adding back @rsmith, @rjmccall.)

In D108479#3113035 , @samitolvanen 
wrote:

> In D108479#3112492 , @rjmccall 
> wrote:
>
>> You could also make this Just Work for things like C++ member functions 
>> rather than producing a member function pointer.
>
> I'm not sure I understand. What does Just Work mean when it comes to C++ 
> member functions?

I think he means that e.g. `__builtin_symbol_address(::bar)` should return 
a `void*` pointing to the address of the `Foo::bar` member function body, 
instead of a member function pointer for `Foo::bar`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108479

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

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

> Maybe it should even be semantically restricted to require a constant decl 
> reference as its operand?

I think we should do this.

Maybe it should be named something like `__builtin_symbol_address` if we're 
intending for this to have an effect with PAuth as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108479

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

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

[Peter Collingbourne via Phabricator via cfe-commits]

pcc requested changes to this revision.
pcc added subscribers: rjmccall, rsmith.
pcc added a comment.
This revision now requires changes to proceed.

In D108479#3108360 , @ardb wrote:

> I would argue that the existing __builtin_addressof() should absorb this 
> behavior, rather than adding a special builtin related to CFI.
>
> As is documented for __builtin_addressof(), its intended use is in cases 
> where the & operator may return something other than the address of the 
> object, which is exactly the case we are dealing with here.

Maybe. It does imply two slightly orthogonal use cases for 
`__builtin_addressof`, one for implementing std::addressof and the other for 
the no-CFI use case. One question is then whether these two use cases will 
conflict. In libc++ we have

  template 
  inline _LIBCPP_CONSTEXPR_AFTER_CXX14
  _LIBCPP_NO_CFI _LIBCPP_INLINE_VISIBILITY
  _Tp*
  addressof(_Tp& __x) _NOEXCEPT
  {
  return __builtin_addressof(__x);
  }

So as long as the additional use case for `__builtin_addressof` is only 
activated when the argument is (syntactically) a reference to a function I 
don't think this libc++ code will be affected. I guess it's possible that there 
could be other users of `__builtin_addressof` that could be relying on the 
existing behavior, although it seems unlikely to me.

A similar use case that we may want to consider is for removing the pointer 
authentication bits when using the proposed PAuth ABI. D112941 
 proposes adding a separate builtin 
`__builtin_ptrauth_strip` for this purpose, although I believe that it cannot 
be used in constant expressions. If we decide to let `__builtin_addressof` 
absorb the no-CFI behavior then it seems reasonable for it to absorb the PAuth 
stripping behavior as well, but again only if the argument is a syntactic 
function reference. I personally would find this somewhat confusing though 
because stripping in the PAuth ABI can be performed at runtime on any pointer 
value and not just syntactic function references.

Should the builtin return a value of type `void*` instead of a pointer to the 
type of its argument? This would make it clear that the value returned by the 
builtin is not meant to be called in the usual way (this applies to both CFI 
and the PAuth ABI) and I suppose is another point in favor of this being a 
separate builtin.

Adding @rsmith and @rjmccall who may have opinions on the above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108479

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

[PATCH] D112761: cfi-icall: Add -fsanitize-cfi-promotion-aliases

[Peter Collingbourne via Phabricator via cfe-commits]

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

I asked @samitolvanen out-of-band to check whether this really needs a flag 
since it seems like there could be some underlying issue that needs to be 
resolved so that we can do this unconditionally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112761

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

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/AST/APValue.cpp:133
+void APValue::LValueBase::setNoCFIValue(bool NoCFI) {
+  if (is()) {
+Local.NoCFIValue = NoCFI;

Remove braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108479

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

[PATCH] D42225: libcxx: Provide overloads for basic_filebuf::open() et al that take wchar_t* filenames on Windows.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added subscribers: thomasanderson, thakis.
pcc added a comment.

I believe that Chromium uses it (or at least it did at the time that I added 
this, and Chromium has since switched to using libc++ on Windows).

I don't work on Chromium much anymore but perhaps @thakis or @thomasanderson 
can confirm.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D42225

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

[PATCH] D107850: [asan] Implemented intrinsic for the custom calling convention similar used by HWASan for X86.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:1640
 
+def int_asan_check_memaccess :
+  Intrinsic<[],[llvm_ptr_ty, llvm_i8_ty, llvm_i8_ty],

kstoimenov wrote:
> eugenis wrote:
> > vitalybuka wrote:
> > > pcc wrote:
> > > > eugenis wrote:
> > > > > vitalybuka wrote:
> > > > > > kstoimenov wrote:
> > > > > > > vitalybuka wrote:
> > > > > > > > vitalybuka wrote:
> > > > > > > > > PTAL at lvm.read_register.i32
> > > > > > > > > 
> > > > > > > > > How about:
> > > > > > > > > 
> > > > > > > > > llvm.asan.check.memaccess ->
> > > > > > > > >   lvm.asan.check_read
> > > > > > > > >   lvm.asan.check_write
> > > > > > > > >   lvm.asan.kernel.check_read
> > > > > > > > >   lvm.asan.kernel.check_write
> > > > > > > > > 
> > > > > > > > > Even better
> > > > > > > > >   lvm.asan.check_read.{i8, i16, i32, ...}
> > > > > > > > >   lvm.asan.check_write.{i8, i16, i32, ...}
> > > > > > > > >   lvm.asan.kernel.check_read.{i8, i16, i32, ...}
> > > > > > > > >   lvm.asan.kernel.check_write.{i8, i16, i32, ...}
> > > > > > > > > 
> > > > > > > > Looks like underscore is not used in intrinsic names, so 
> > > > > > > > essentially the same with dots.
> > > > > > > Sounds good to me. I do the full expansion so there will be 20 
> > > > > > > intrinsics altogether. I will update the code and ping you when 
> > > > > > > done. 
> > > > > > @pcc @eugenis 
> > > > > > WDYT, I think later we can do the same for HWASAN?
> > > > > I don't see what these multiple intrinsics give us that a single 
> > > > > memaccess one does not provide?
> > > > > 
> > > > > As long as access type and similar arguments are immediates.
> > > > > 
> > > > Agree with @eugenis - these sorts of intrinsic variants are typically 
> > > > used for distinguishing different pointer element types and we're in 
> > > > the process of getting rid of those anyway.
> > > @pcc @eugenis Then do you prefer to encode is_write+size+kernel into 
> > > non-human unreadable AccessInfo, like hwasan, or separate 0/1 arguments.
> > > I probably prefer AccessInfo, as they both unreadable, but the hwasan 
> > > version is shorter.
> > don't have a strong opinion, but sometimes I wish that hwasan outlined 
> > function names were more readable. The magic number in the names takes 
> > effort to decode.
> > 
> I think I am gonna go with int_asan_check(?_kernel)_(load|store) and pass the 
> size as parameter. What do you think? 
You mean in a register? I think that could mean more register pressure -> 
higher code size.

The magic numbers are unfortunate but they aren't that hard to decode (maybe we 
should be printing them as hex to make it a bit easier). I suppose we could 
pretty print the access info into the symbol name but only a few people will be 
looking at these so I'm not sure it's worth the effort.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107850

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

[PATCH] D107850: [asan] Implemented intrinsic for the custom calling convention similar used by HWASan for X86.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:1640
 
+def int_asan_check_memaccess :
+  Intrinsic<[],[llvm_ptr_ty, llvm_i8_ty, llvm_i8_ty],

eugenis wrote:
> vitalybuka wrote:
> > kstoimenov wrote:
> > > vitalybuka wrote:
> > > > vitalybuka wrote:
> > > > > PTAL at lvm.read_register.i32
> > > > > 
> > > > > How about:
> > > > > 
> > > > > llvm.asan.check.memaccess ->
> > > > >   lvm.asan.check_read
> > > > >   lvm.asan.check_write
> > > > >   lvm.asan.kernel.check_read
> > > > >   lvm.asan.kernel.check_write
> > > > > 
> > > > > Even better
> > > > >   lvm.asan.check_read.{i8, i16, i32, ...}
> > > > >   lvm.asan.check_write.{i8, i16, i32, ...}
> > > > >   lvm.asan.kernel.check_read.{i8, i16, i32, ...}
> > > > >   lvm.asan.kernel.check_write.{i8, i16, i32, ...}
> > > > > 
> > > > Looks like underscore is not used in intrinsic names, so essentially 
> > > > the same with dots.
> > > Sounds good to me. I do the full expansion so there will be 20 intrinsics 
> > > altogether. I will update the code and ping you when done. 
> > @pcc @eugenis 
> > WDYT, I think later we can do the same for HWASAN?
> I don't see what these multiple intrinsics give us that a single memaccess 
> one does not provide?
> 
> As long as access type and similar arguments are immediates.
> 
Agree with @eugenis - these sorts of intrinsic variants are typically used for 
distinguishing different pointer element types and we're in the process of 
getting rid of those anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107850

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

[PATCH] D108223: gn build: Build libclang.so on ELF platforms.

[Peter Collingbourne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb2e77cd095a6: gn build: Build libclang.so and libLTO.so on 
ELF platforms. (authored by pcc).

Changed prior to commit:
  https://reviews.llvm.org/D108223?vs=366966=367318#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108223

Files:
  llvm/utils/gn/build/BUILD.gn
  llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
  llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn
@@ -1,20 +1,14 @@
 import("//llvm/utils/gn/build/symbol_exports.gni")
 import("//llvm/version.gni")
 
-lto_target_type = "shared_library"
-if (host_os != "mac" && host_os != "win") {
-  # ELF targets need -fPIC to build shared libs but they aren't on by default.
-  # For now, make libclang a static lib there.
-  lto_target_type = "static_library"
-}
-
 symbol_exports("exports") {
   exports_file = "lto.exports"
 }
 
-target(lto_target_type, "lto") {
+shared_library("lto") {
   output_name = "LTO"
   deps = [
+":exports",
 "//llvm/lib/Bitcode/Reader",
 "//llvm/lib/IR",
 "//llvm/lib/LTO",
@@ -29,10 +23,6 @@
 "lto.cpp",
   ]
 
-  if (lto_target_type == "shared_library") {
-deps += [ ":exports" ]
-  }
-
   if (host_os == "mac") {
 ldflags = [
   "-Wl,-compatibility_version,1",
Index: llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
+++ llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
@@ -5,33 +5,24 @@
 # This build file is just enough to get check-clang to pass, it's missing
 # several things from the CMake build:
 # - a build target copying the Python bindings
-# - the GN linux build always builds without -fPIC (as if LLVM_ENABLE_PIC=OFF
-#   in the CMake build), so libclang is always a static library on linux
 # - the GN build doesn't have LIBCLANG_BUILD_STATIC
 
-libclang_target_type = "shared_library"
-if (host_os != "win" && host_os != "mac") {
-  # ELF targets need -fPIC to build shared libs but they aren't on by default.
-  # For now, make libclang a static lib there.
-  libclang_target_type = "static_library"
-} else {
-  action("linker_script_to_exports") {
-script = "linker-script-to-export-list.py"
-inputs = [ "libclang.map" ]
-outputs = [ "$target_gen_dir/libclang.exports" ]
-args = [
-  rebase_path(inputs[0], root_build_dir),
-  rebase_path(outputs[0], root_build_dir),
-]
-  }
+action("linker_script_to_exports") {
+  script = "linker-script-to-export-list.py"
+  inputs = [ "libclang.map" ]
+  outputs = [ "$target_gen_dir/libclang.exports" ]
+  args = [
+rebase_path(inputs[0], root_build_dir),
+rebase_path(outputs[0], root_build_dir),
+  ]
+}
 
-  symbol_exports("exports") {
-deps = [ ":linker_script_to_exports" ]
-exports_file = "$target_gen_dir/libclang.exports"
-  }
+symbol_exports("exports") {
+  deps = [ ":linker_script_to_exports" ]
+  exports_file = "$target_gen_dir/libclang.exports"
 }
 
-target(libclang_target_type, "libclang") {
+shared_library("libclang") {
   configs += [ "//llvm/utils/gn/build:clang_code" ]
   deps = [
 "//clang/include/clang/Config",
@@ -48,14 +39,17 @@
 "//llvm/lib/Support",
 "//llvm/lib/Target:TargetsToBuild",
   ]
+  if (current_os == "win" || current_os == "mac") {
+deps += [ ":exports" ]
+  } else {
+inputs = [ "libclang.map" ]
+ldflags =
+[ "-Wl,--version-script," + rebase_path(inputs[0], root_build_dir) ]
+  }
   if (clang_enable_arcmt) {
 deps += [ "//clang/lib/ARCMigrate" ]
   }
 
-  if (libclang_target_type == "shared_library") {
-deps += [ ":exports" ]
-  }
-
   defines = []
 
   if (host_os == "win") {
Index: llvm/utils/gn/build/BUILD.gn
===
--- llvm/utils/gn/build/BUILD.gn
+++ llvm/utils/gn/build/BUILD.gn
@@ -377,6 +377,9 @@
 "//llvm/include",
 "$root_gen_dir/llvm/include",
   ]
+  if (current_os != "win") {
+cflags = [ "-fPIC" ]
+  }
 }
 
 config("lld_code") {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108223: gn build: Build libclang.so on ELF platforms.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn:16
-  # For now, make libclang a static lib there.
-  libclang_target_type = "static_library"
-} else {

thakis wrote:
> probably should update these too:
> 
> ```
> % rg -F 'rget_type = "static_librar' llvm/utils/gn
> llvm/utils/gn/secondary/compiler-rt/lib/tsan/BUILD.gn
> 12:  tsan_target_type = "static_library"
> 
> llvm/utils/gn/secondary/compiler-rt/lib/asan/BUILD.gn
> 12:  asan_target_type = "static_library"
> 
> llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
> 16:  libclang_target_type = "static_library"
> 
> llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn
> 8:  lto_target_type = "static_library"
> ```
Done for llvm/tools/lto.

For the sanitizers the shared-ness of the runtime is tied to what works best on 
that platform, rather than whether it's PIC by default (note that all 
compiler-rt code is built with -fPIC), so I left them as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108223

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

[PATCH] D108223: gn build: Build libclang.so on ELF platforms.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc updated this revision to Diff 366966.
pcc added a comment.
Herald added subscribers: ormris, steven_wu, hiraditya.

Also libLTO.so


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108223

Files:
  llvm/utils/gn/build/BUILD.gn
  llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
  llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/tools/lto/BUILD.gn
@@ -1,18 +1,11 @@
 import("//llvm/utils/gn/build/symbol_exports.gni")
 import("//llvm/version.gni")
 
-lto_target_type = "shared_library"
-if (host_os != "mac" && host_os != "win") {
-  # ELF targets need -fPIC to build shared libs but they aren't on by default.
-  # For now, make libclang a static lib there.
-  lto_target_type = "static_library"
-}
-
 symbol_exports("exports") {
   exports_file = "lto.exports"
 }
 
-target(lto_target_type, "lto") {
+shared_library("lto") {
   output_name = "LTO"
   deps = [
 "//llvm/lib/Bitcode/Reader",
@@ -23,16 +16,13 @@
 "//llvm/lib/Support",
 "//llvm/lib/Target",
 "//llvm/lib/Target:TargetsToBuild",
+":exports",
   ]
   sources = [
 "LTODisassembler.cpp",
 "lto.cpp",
   ]
 
-  if (lto_target_type == "shared_library") {
-deps += [ ":exports" ]
-  }
-
   if (host_os == "mac") {
 ldflags = [
   "-Wl,-compatibility_version,1",
Index: llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
+++ llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
@@ -5,33 +5,24 @@
 # This build file is just enough to get check-clang to pass, it's missing
 # several things from the CMake build:
 # - a build target copying the Python bindings
-# - the GN linux build always builds without -fPIC (as if LLVM_ENABLE_PIC=OFF
-#   in the CMake build), so libclang is always a static library on linux
 # - the GN build doesn't have LIBCLANG_BUILD_STATIC
 
-libclang_target_type = "shared_library"
-if (host_os != "win" && host_os != "mac") {
-  # ELF targets need -fPIC to build shared libs but they aren't on by default.
-  # For now, make libclang a static lib there.
-  libclang_target_type = "static_library"
-} else {
-  action("linker_script_to_exports") {
-script = "linker-script-to-export-list.py"
-inputs = [ "libclang.map" ]
-outputs = [ "$target_gen_dir/libclang.exports" ]
-args = [
-  rebase_path(inputs[0], root_build_dir),
-  rebase_path(outputs[0], root_build_dir),
-]
-  }
+action("linker_script_to_exports") {
+  script = "linker-script-to-export-list.py"
+  inputs = [ "libclang.map" ]
+  outputs = [ "$target_gen_dir/libclang.exports" ]
+  args = [
+rebase_path(inputs[0], root_build_dir),
+rebase_path(outputs[0], root_build_dir),
+  ]
+}
 
-  symbol_exports("exports") {
-deps = [ ":linker_script_to_exports" ]
-exports_file = "$target_gen_dir/libclang.exports"
-  }
+symbol_exports("exports") {
+  deps = [ ":linker_script_to_exports" ]
+  exports_file = "$target_gen_dir/libclang.exports"
 }
 
-target(libclang_target_type, "libclang") {
+shared_library("libclang") {
   configs += [ "//llvm/utils/gn/build:clang_code" ]
   deps = [
 "//clang/include/clang/Config",
@@ -48,14 +39,17 @@
 "//llvm/lib/Support",
 "//llvm/lib/Target:TargetsToBuild",
   ]
+  if (current_os == "win" || current_os == "mac") {
+deps += [ ":exports" ]
+  } else {
+inputs = [ "libclang.map" ]
+ldflags =
+[ "-Wl,--version-script," + rebase_path(inputs[0], root_build_dir) ]
+  }
   if (clang_enable_arcmt) {
 deps += [ "//clang/lib/ARCMigrate" ]
   }
 
-  if (libclang_target_type == "shared_library") {
-deps += [ ":exports" ]
-  }
-
   defines = []
 
   if (host_os == "win") {
Index: llvm/utils/gn/build/BUILD.gn
===
--- llvm/utils/gn/build/BUILD.gn
+++ llvm/utils/gn/build/BUILD.gn
@@ -377,6 +377,9 @@
 "//llvm/include",
 "$root_gen_dir/llvm/include",
   ]
+  if (current_os != "win") {
+cflags = [ "-fPIC" ]
+  }
 }
 
 config("lld_code") {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108223: gn build: Build libclang.so on ELF platforms.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added a reviewer: thakis.
pcc requested review of this revision.
Herald added a project: LLVM.

This requires changing the ELF build to enable -fPIC, consistent
with other platforms.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108223

Files:
  llvm/utils/gn/build/BUILD.gn
  llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn


Index: llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
+++ llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
@@ -5,33 +5,24 @@
 # This build file is just enough to get check-clang to pass, it's missing
 # several things from the CMake build:
 # - a build target copying the Python bindings
-# - the GN linux build always builds without -fPIC (as if LLVM_ENABLE_PIC=OFF
-#   in the CMake build), so libclang is always a static library on linux
 # - the GN build doesn't have LIBCLANG_BUILD_STATIC
 
-libclang_target_type = "shared_library"
-if (host_os != "win" && host_os != "mac") {
-  # ELF targets need -fPIC to build shared libs but they aren't on by default.
-  # For now, make libclang a static lib there.
-  libclang_target_type = "static_library"
-} else {
-  action("linker_script_to_exports") {
-script = "linker-script-to-export-list.py"
-inputs = [ "libclang.map" ]
-outputs = [ "$target_gen_dir/libclang.exports" ]
-args = [
-  rebase_path(inputs[0], root_build_dir),
-  rebase_path(outputs[0], root_build_dir),
-]
-  }
+action("linker_script_to_exports") {
+  script = "linker-script-to-export-list.py"
+  inputs = [ "libclang.map" ]
+  outputs = [ "$target_gen_dir/libclang.exports" ]
+  args = [
+rebase_path(inputs[0], root_build_dir),
+rebase_path(outputs[0], root_build_dir),
+  ]
+}
 
-  symbol_exports("exports") {
-deps = [ ":linker_script_to_exports" ]
-exports_file = "$target_gen_dir/libclang.exports"
-  }
+symbol_exports("exports") {
+  deps = [ ":linker_script_to_exports" ]
+  exports_file = "$target_gen_dir/libclang.exports"
 }
 
-target(libclang_target_type, "libclang") {
+shared_library("libclang") {
   configs += [ "//llvm/utils/gn/build:clang_code" ]
   deps = [
 "//clang/include/clang/Config",
@@ -48,14 +39,17 @@
 "//llvm/lib/Support",
 "//llvm/lib/Target:TargetsToBuild",
   ]
+  if (current_os == "win" || current_os == "mac") {
+deps += [ ":exports" ]
+  } else {
+inputs = [ "libclang.map" ]
+ldflags =
+[ "-Wl,--version-script," + rebase_path(inputs[0], root_build_dir) ]
+  }
   if (clang_enable_arcmt) {
 deps += [ "//clang/lib/ARCMigrate" ]
   }
 
-  if (libclang_target_type == "shared_library") {
-deps += [ ":exports" ]
-  }
-
   defines = []
 
   if (host_os == "win") {
Index: llvm/utils/gn/build/BUILD.gn
===
--- llvm/utils/gn/build/BUILD.gn
+++ llvm/utils/gn/build/BUILD.gn
@@ -377,6 +377,9 @@
 "//llvm/include",
 "$root_gen_dir/llvm/include",
   ]
+  if (current_os != "win") {
+cflags = [ "-fPIC" ]
+  }
 }
 
 config("lld_code") {


Index: llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
+++ llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn
@@ -5,33 +5,24 @@
 # This build file is just enough to get check-clang to pass, it's missing
 # several things from the CMake build:
 # - a build target copying the Python bindings
-# - the GN linux build always builds without -fPIC (as if LLVM_ENABLE_PIC=OFF
-#   in the CMake build), so libclang is always a static library on linux
 # - the GN build doesn't have LIBCLANG_BUILD_STATIC
 
-libclang_target_type = "shared_library"
-if (host_os != "win" && host_os != "mac") {
-  # ELF targets need -fPIC to build shared libs but they aren't on by default.
-  # For now, make libclang a static lib there.
-  libclang_target_type = "static_library"
-} else {
-  action("linker_script_to_exports") {
-script = "linker-script-to-export-list.py"
-inputs = [ "libclang.map" ]
-outputs = [ "$target_gen_dir/libclang.exports" ]
-args = [
-  rebase_path(inputs[0], root_build_dir),
-  rebase_path(outputs[0], root_build_dir),
-]
-  }
+action("linker_script_to_exports") {
+  script = "linker-script-to-export-list.py"
+  inputs = [ "libclang.map" ]
+  outputs = [ "$target_gen_dir/libclang.exports" ]
+  args = [
+rebase_path(inputs[0], root_build_dir),
+rebase_path(outputs[0], root_build_dir),
+  ]
+}
 
-  symbol_exports("exports") {
-deps = [ ":linker_script_to_exports" ]
-exports_file = "$target_gen_dir/libclang.exports"
-  }
+symbol_exports("exports") {
+  deps = [ ":linker_script_to_exports" ]
+  exports_file = "$target_gen_dir/libclang.exports"
 }
 
-target(libclang_target_type, "libclang") {
+shared_library("libclang") {
   configs 

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

[Peter Collingbourne via Phabricator via cfe-commits]

pcc accepted this revision.
pcc added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104058

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.
Herald added a subscriber: ormris.



Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:73
+  return false;
+
+// If an operand in the lookup table is not dso_local,

In the version of the patch that you committed, you have this check here:
```
// If operand is mutable, do not generate a relative lookup table.
auto *GlovalVarOp = dyn_cast(GVOp);
if (!GlovalVarOp || !GlovalVarOp->isConstant())
  return false;
```
1. Nit: Gloval -> Global
2. Why is it important whether the referenced global is mutable? The pointer 
itself is constant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

[Peter Collingbourne via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe655e74a318e: AST: Create __va_list in the std namespace 
even in C. (authored by pcc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104830

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/arm64-be-hfa-vararg.c
  clang/test/CodeGen/cfi-icall-va-list.c
  clang/test/Headers/stdarg.cpp

Index: clang/test/Headers/stdarg.cpp
===
--- clang/test/Headers/stdarg.cpp
+++ clang/test/Headers/stdarg.cpp
@@ -15,7 +15,7 @@
 
 #include 
 
-// AARCH64-C: define {{.*}} @f(i32 %n, %struct.__va_list* %list)
+// AARCH64-C: define {{.*}} @f(i32 %n, %"struct.std::__va_list"* %list)
 // AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 %n, %"struct.std::__va_list"* %list)
 // X86_64-C: define {{.*}} @f(i32 %n, %struct.__va_list_tag* %list)
 // X86_64-CXX: define {{.*}} @_Z1fiP13__va_list_tag(i32 %n, %struct.__va_list_tag* %list)
Index: clang/test/CodeGen/cfi-icall-va-list.c
===
--- /dev/null
+++ clang/test/CodeGen/cfi-icall-va-list.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -triple aarch64-unknown-linux -fsanitize=cfi-icall -fsanitize-trap=cfi-icall -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: define dso_local void @f({{.*}} !type [[TYPE:![0-9]+]] !type [[TYPE_GENERALIZED:![0-9]+]]
+void f(__builtin_va_list l) {}
+
+// CHECK-DAG: [[TYPE]] = !{i64 0, !"_ZTSFvSt9__va_listE"}
+// CHECK-DAG: [[TYPE_GENERALIZED]] = !{i64 0, !"_ZTSFvSt9__va_listE.generalized"}
Index: clang/test/CodeGen/arm64-be-hfa-vararg.c
===
--- clang/test/CodeGen/arm64-be-hfa-vararg.c
+++ clang/test/CodeGen/arm64-be-hfa-vararg.c
@@ -4,12 +4,12 @@
 
 // A single member HFA must be aligned just like a non-HFA register argument.
 double callee(int a, ...) {
-// CHECK: [[REGPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 2
+// CHECK: [[REGPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 2
 // CHECK: [[REGP:%.*]] = load i8*, i8** [[REGPP]], align 8
 // CHECK: [[OFFSET0:%.*]] = getelementptr inbounds i8, i8* [[REGP]], i32 {{.*}}
 // CHECK: [[OFFSET1:%.*]] = getelementptr inbounds i8, i8* [[OFFSET0]], i64 8
 
-// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 0
+// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 0
 // CHECK: [[MEMP:%.*]] = load i8*, i8** [[MEMPP]], align 8
 // CHECK: [[NEXTP:%.*]] = getelementptr inbounds i8, i8* [[MEMP]], i64 8
 // CHECK: store i8* [[NEXTP]], i8** [[MEMPP]], align 8
Index: clang/test/CodeGen/aarch64-varargs.c
===
--- clang/test/CodeGen/aarch64-varargs.c
+++ clang/test/CodeGen/aarch64-varargs.c
@@ -11,18 +11,18 @@
 int simple_int(void) {
 // CHECK-LABEL: define{{.*}} i32 @simple_int
   return va_arg(the_list, int);
-// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
 // CHECK: [[EARLY_ONSTACK:%[a-z_0-9]+]] = icmp sge i32 [[GR_OFFS]], 0
 // CHECK: br i1 [[EARLY_ONSTACK]], label %[[VAARG_ON_STACK:[a-z_.0-9]+]], label %[[VAARG_MAYBE_REG:[a-z_.0-9]+]]
 
 // CHECK: [[VAARG_MAYBE_REG]]
 // CHECK: [[NEW_REG_OFFS:%[a-z_0-9]+]] = add i32 [[GR_OFFS]], 8
-// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
+// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
 // CHECK: [[INREG:%[a-z_0-9]+]] = icmp sle i32 [[NEW_REG_OFFS]], 0
 // CHECK: br i1 [[INREG]], label %[[VAARG_IN_REG:[a-z_.0-9]+]], label %[[VAARG_ON_STACK]]
 
 // CHECK: [[VAARG_IN_REG]]
-// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 1)
+// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1)
 // CHECK: [[REG_ADDR:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[REG_TOP]], i32 [[GR_OFFS]]
 // CHECK-BE: [[REG_ADDR_ALIGNED:%[0-9]+]] = getelementptr inbounds i8, i8* [[REG_ADDR]], i64 4
 // CHECK-BE: [[FROMREG_ADDR:%[a-z_0-9]+]] = bitcast i8* [[REG_ADDR_ALIGNED]] to 

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc updated this revision to Diff 354130.
pcc added a comment.

Add test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104830

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/arm64-be-hfa-vararg.c
  clang/test/CodeGen/cfi-icall-va-list.c
  clang/test/Headers/stdarg.cpp

Index: clang/test/Headers/stdarg.cpp
===
--- clang/test/Headers/stdarg.cpp
+++ clang/test/Headers/stdarg.cpp
@@ -15,7 +15,7 @@
 
 #include 
 
-// AARCH64-C: define {{.*}} @f(i32 %n, %struct.__va_list* %list)
+// AARCH64-C: define {{.*}} @f(i32 %n, %"struct.std::__va_list"* %list)
 // AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 %n, %"struct.std::__va_list"* %list)
 // X86_64-C: define {{.*}} @f(i32 %n, %struct.__va_list_tag* %list)
 // X86_64-CXX: define {{.*}} @_Z1fiP13__va_list_tag(i32 %n, %struct.__va_list_tag* %list)
Index: clang/test/CodeGen/cfi-icall-va-list.c
===
--- /dev/null
+++ clang/test/CodeGen/cfi-icall-va-list.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -triple aarch64-unknown-linux -fsanitize=cfi-icall -fsanitize-trap=cfi-icall -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: define dso_local void @f({{.*}} !type [[TYPE:![0-9]+]] !type [[TYPE_GENERALIZED:![0-9]+]]
+void f(__builtin_va_list l) {}
+
+// CHECK-DAG: [[TYPE]] = !{i64 0, !"_ZTSFvSt9__va_listE"}
+// CHECK-DAG: [[TYPE_GENERALIZED]] = !{i64 0, !"_ZTSFvSt9__va_listE.generalized"}
Index: clang/test/CodeGen/arm64-be-hfa-vararg.c
===
--- clang/test/CodeGen/arm64-be-hfa-vararg.c
+++ clang/test/CodeGen/arm64-be-hfa-vararg.c
@@ -4,12 +4,12 @@
 
 // A single member HFA must be aligned just like a non-HFA register argument.
 double callee(int a, ...) {
-// CHECK: [[REGPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 2
+// CHECK: [[REGPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 2
 // CHECK: [[REGP:%.*]] = load i8*, i8** [[REGPP]], align 8
 // CHECK: [[OFFSET0:%.*]] = getelementptr inbounds i8, i8* [[REGP]], i32 {{.*}}
 // CHECK: [[OFFSET1:%.*]] = getelementptr inbounds i8, i8* [[OFFSET0]], i64 8
 
-// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 0
+// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 0
 // CHECK: [[MEMP:%.*]] = load i8*, i8** [[MEMPP]], align 8
 // CHECK: [[NEXTP:%.*]] = getelementptr inbounds i8, i8* [[MEMP]], i64 8
 // CHECK: store i8* [[NEXTP]], i8** [[MEMPP]], align 8
Index: clang/test/CodeGen/aarch64-varargs.c
===
--- clang/test/CodeGen/aarch64-varargs.c
+++ clang/test/CodeGen/aarch64-varargs.c
@@ -11,18 +11,18 @@
 int simple_int(void) {
 // CHECK-LABEL: define{{.*}} i32 @simple_int
   return va_arg(the_list, int);
-// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
 // CHECK: [[EARLY_ONSTACK:%[a-z_0-9]+]] = icmp sge i32 [[GR_OFFS]], 0
 // CHECK: br i1 [[EARLY_ONSTACK]], label %[[VAARG_ON_STACK:[a-z_.0-9]+]], label %[[VAARG_MAYBE_REG:[a-z_.0-9]+]]
 
 // CHECK: [[VAARG_MAYBE_REG]]
 // CHECK: [[NEW_REG_OFFS:%[a-z_0-9]+]] = add i32 [[GR_OFFS]], 8
-// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
+// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
 // CHECK: [[INREG:%[a-z_0-9]+]] = icmp sle i32 [[NEW_REG_OFFS]], 0
 // CHECK: br i1 [[INREG]], label %[[VAARG_IN_REG:[a-z_.0-9]+]], label %[[VAARG_ON_STACK]]
 
 // CHECK: [[VAARG_IN_REG]]
-// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 1)
+// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1)
 // CHECK: [[REG_ADDR:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[REG_TOP]], i32 [[GR_OFFS]]
 // CHECK-BE: [[REG_ADDR_ALIGNED:%[0-9]+]] = getelementptr inbounds i8, i8* [[REG_ADDR]], i64 4
 // CHECK-BE: [[FROMREG_ADDR:%[a-z_0-9]+]] = bitcast i8* [[REG_ADDR_ALIGNED]] to i32*
@@ -30,9 +30,9 @@
 // CHECK: br label %[[VAARG_END:[a-z._0-9]+]]
 
 // CHECK: [[VAARG_ON_STACK]]
-// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 0)
+// 

[PATCH] D104830: AST: Create __va_list in the std namespace even in C.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: rsmith, eugenis.
pcc requested review of this revision.
Herald added a project: clang.

This ensures that the mangled type names match between C and C++,
which is significant when using -fsanitize=cfi-icall. Ideally we
wouldn't have created this namespace at all, but it's now part of
the ABI (e.g. in mangled names), so we can't change it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104830

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/arm64-be-hfa-vararg.c
  clang/test/Headers/stdarg.cpp

Index: clang/test/Headers/stdarg.cpp
===
--- clang/test/Headers/stdarg.cpp
+++ clang/test/Headers/stdarg.cpp
@@ -15,7 +15,7 @@
 
 #include 
 
-// AARCH64-C: define {{.*}} @f(i32 %n, %struct.__va_list* %list)
+// AARCH64-C: define {{.*}} @f(i32 %n, %"struct.std::__va_list"* %list)
 // AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 %n, %"struct.std::__va_list"* %list)
 // X86_64-C: define {{.*}} @f(i32 %n, %struct.__va_list_tag* %list)
 // X86_64-CXX: define {{.*}} @_Z1fiP13__va_list_tag(i32 %n, %struct.__va_list_tag* %list)
Index: clang/test/CodeGen/arm64-be-hfa-vararg.c
===
--- clang/test/CodeGen/arm64-be-hfa-vararg.c
+++ clang/test/CodeGen/arm64-be-hfa-vararg.c
@@ -4,12 +4,12 @@
 
 // A single member HFA must be aligned just like a non-HFA register argument.
 double callee(int a, ...) {
-// CHECK: [[REGPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 2
+// CHECK: [[REGPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 2
 // CHECK: [[REGP:%.*]] = load i8*, i8** [[REGPP]], align 8
 // CHECK: [[OFFSET0:%.*]] = getelementptr inbounds i8, i8* [[REGP]], i32 {{.*}}
 // CHECK: [[OFFSET1:%.*]] = getelementptr inbounds i8, i8* [[OFFSET0]], i64 8
 
-// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 0
+// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 0
 // CHECK: [[MEMP:%.*]] = load i8*, i8** [[MEMPP]], align 8
 // CHECK: [[NEXTP:%.*]] = getelementptr inbounds i8, i8* [[MEMP]], i64 8
 // CHECK: store i8* [[NEXTP]], i8** [[MEMPP]], align 8
Index: clang/test/CodeGen/aarch64-varargs.c
===
--- clang/test/CodeGen/aarch64-varargs.c
+++ clang/test/CodeGen/aarch64-varargs.c
@@ -11,18 +11,18 @@
 int simple_int(void) {
 // CHECK-LABEL: define{{.*}} i32 @simple_int
   return va_arg(the_list, int);
-// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
 // CHECK: [[EARLY_ONSTACK:%[a-z_0-9]+]] = icmp sge i32 [[GR_OFFS]], 0
 // CHECK: br i1 [[EARLY_ONSTACK]], label %[[VAARG_ON_STACK:[a-z_.0-9]+]], label %[[VAARG_MAYBE_REG:[a-z_.0-9]+]]
 
 // CHECK: [[VAARG_MAYBE_REG]]
 // CHECK: [[NEW_REG_OFFS:%[a-z_0-9]+]] = add i32 [[GR_OFFS]], 8
-// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
+// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
 // CHECK: [[INREG:%[a-z_0-9]+]] = icmp sle i32 [[NEW_REG_OFFS]], 0
 // CHECK: br i1 [[INREG]], label %[[VAARG_IN_REG:[a-z_.0-9]+]], label %[[VAARG_ON_STACK]]
 
 // CHECK: [[VAARG_IN_REG]]
-// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 1)
+// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1)
 // CHECK: [[REG_ADDR:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[REG_TOP]], i32 [[GR_OFFS]]
 // CHECK-BE: [[REG_ADDR_ALIGNED:%[0-9]+]] = getelementptr inbounds i8, i8* [[REG_ADDR]], i64 4
 // CHECK-BE: [[FROMREG_ADDR:%[a-z_0-9]+]] = bitcast i8* [[REG_ADDR_ALIGNED]] to i32*
@@ -30,9 +30,9 @@
 // CHECK: br label %[[VAARG_END:[a-z._0-9]+]]
 
 // CHECK: [[VAARG_ON_STACK]]
-// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 0)
+// CHECK: [[STACK:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 0)
 // CHECK: [[NEW_STACK:%[a-z_0-9]+]] = getelementptr inbounds i8, i8* [[STACK]], i64 8
-// CHECK: store i8* [[NEW_STACK]], i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 0)
+// CHECK: store i8* 

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/test/CodeGen/thinlto-cfi-icall-static-inline-asm.c:3
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -flto=thin 
-fsanitize=cfi-icall -fsplit-lto-unit -o - %s | llvm-dis - | FileCheck %s
+

Can the test be moved to `llvm/test/Transforms/ThinLTOBitcodeWriter` and made 
to use `opt -thinlto-bc`?



Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:79
+  GlobalValue::InternalLinkage, Name, F, );
+  appendToCompilerUsed(ExportM, A);
+}

samitolvanen wrote:
> Note that I'm adding the alias to llvm.compiler.used because it's otherwise 
> removed during optimization. Is there are better way to accomplish this?
Not as far as I know, that's what I'd recommend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104058

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

[PATCH] D103845: [compiler-rt][hwasan] Add newline between record_addr lines on frame record dumps

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103845

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

[PATCH] D103845: [compiler-rt][hwasan] Add newline between record_addr lines on frame record dumps

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Thanks for tracking this down. It seems best to change the Printf call to add 
the newline, since otherwise it looks like you'd be adding a spurious newline 
to the other callers of `RenderFrame` on Fuchsia.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103845

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

[PATCH] D103845: [compiler-rt][hwasan] Add newline between record_addr lines on frame record dumps

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D103845#2806211 , @leonardchan 
wrote:

> In D103845#2804441 , @pcc wrote:
>
>> This isn't how the output looks on Android. Are you sure this isn't a 
>> Fuchsia-specific bug in the output formatting?
>
> I think the newline gets added to the frame description only if 
> `Symbolizer::GetOrInit()->SymbolizePC(pc)` is nonnull, so if it fails then no 
> newline is added.

I see. I guess we never tested the code path where `SymbolizePC` returned null 
on Android.

I think I would prefer doing this in a slightly different way where the `\n` 
gets added in the Printf call on line 244. Then you can remove the `\n` on line 
239 and you shouldn't need the else clause.

Where does the `{{{bt:0:0x214178819a54}}}`come from on Fuchsia? Is it printed 
to the console as a side effect of calling `SymbolizePC`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103845

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

[PATCH] D103845: [compiler-rt][hwasan] Add newline between record_addr lines on frame record dumps

[Peter Collingbourne via Phabricator via cfe-commits]

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

This isn't how the output looks on Android. Are you sure this isn't a 
Fuchsia-specific bug in the output formatting?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103845

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

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Isn't the bug here that module hashing is using `hash_code`? So shouldn't the 
correct fix be to use a specific hashing algorithm for module hashes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102943

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

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

This warning seems to have a lot of false positives on things like reference 
arguments that are used as output parameters. For example here is a small 
sample of output from a stage2 build of part of LLVM:

  In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:
  In file included from ../llvm/include/llvm/BinaryFormat/Minidump.h:21:
  In file included from ../llvm/include/llvm/ADT/BitmaskEnum.h:16:
  ../llvm/include/llvm/Support/MathExtras.h:822:9: warning: variable 
'Overflowed' set but not used [-Wunused-but-set-variable]
bool  = ResultOverflowed ? *ResultOverflowed : Dummy;
  ^
  ../llvm/include/llvm/Support/MathExtras.h:936:72: warning: parameter 'Result' 
set but not used [-Wunused-but-set-parameter]
  std::enable_if_t::value, T> MulOverflow(T X, T Y, T 
) {
 ^
  In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:
  In file included from ../llvm/include/llvm/BinaryFormat/Minidump.h:22:
  In file included from ../llvm/include/llvm/ADT/DenseMapInfo.h:20:
  ../llvm/include/llvm/ADT/StringRef.h:511:37: warning: parameter 'Result' set 
but not used [-Wunused-but-set-parameter]
  getAsInteger(unsigned Radix, T ) const {
  ^
  ../llvm/include/llvm/ADT/StringRef.h:522:37: warning: parameter 'Result' set 
but not used [-Wunused-but-set-parameter]
  getAsInteger(unsigned Radix, T ) const {
  ^
  ../llvm/include/llvm/ADT/StringRef.h:545:39: warning: parameter 'Result' set 
but not used [-Wunused-but-set-parameter]
  consumeInteger(unsigned Radix, T ) {
^
  ../llvm/include/llvm/ADT/StringRef.h:556:39: warning: parameter 'Result' set 
but not used [-Wunused-but-set-parameter]
  consumeInteger(unsigned Radix, T ) {
^
  6 warnings generated.

Could you please take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

This causes us to reject the following (reduced from AOSP):

  int sprintf(char* __s, const char* __fmt, ...)
  __attribute__((__format__(printf, 2, 3))) ;
  int sprintf(char* dest, const char* format)
  __attribute__((overloadable))
  __attribute__((enable_if(((__builtin_object_size(((dest)), (1))) != 
((unsigned long) -1) && (__builtin_object_size(((dest)), (1))) < 
(__builtin_strlen(format))), "format string will always overflow destination 
buffer")))
  
  __attribute__((unavailable("format string will always overflow 
destination buffer")));
  
  void f() {
unsigned char number_buffer[26] = {0};
sprintf((char *)number_buffer, "null");
  }

It doesn't seem like we ought to be rejecting this case. Can you please take a 
look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

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

[PATCH] D63908: hwasan: Improve precision of checks using short granule tags.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: compiler-rt/trunk/lib/hwasan/hwasan_checks.h:76
+#endif
+  return *(u8 *)(ptr | (kShadowAlignment - 1)) == ptr_tag;
+}

xiangzhangllvm wrote:
> Hello @pcc I think here seems some problem, the ptr is user passing point,
> *(ptr + n) should have the user's real data. it shouldn't  "== ptr_tag".
If this is a short granule then from the user's perspective the maximum size of 
the granule is (granule size - 1). This means that the last byte of the granule 
is free for us to use to store the granule's real tag.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63908

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

[PATCH] D91466: [WIP][clang][Fuchsia] Support HWASan for Fuchsia

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

How does Zircon handle tagged addresses in syscalls? Are they handled 
equivalently to Linux's tagged address ABI?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91466

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

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

I agree with @MaskRay that this should be a binutils-specific option. The flag 
`-mlinker-version` seems to have been designed around macOS-specific 
assumptions i.e. there is a single linker (ld64) and that the linker and 
assembler are not version coupled. Having this option be binutils-specific 
seems like the best way to reflect the binutils-specific requirements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85474

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

[PATCH] D90424: AArch64: Use SBFX instead of UBFX to extract address granule in outlined HWASan checks.

[Peter Collingbourne 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 rGc9b1a2b41dca: AArch64: Use SBFX instead of UBFX to extract 
address granule in outlined HWASan… (authored by pcc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90424

Files:
  clang/docs/HardwareAssistedAddressSanitizerDesign.rst
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll


Index: llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
===
--- llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
+++ llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
@@ -38,7 +38,7 @@
 ; CHECK-NEXT: .weak __hwasan_check_x0_2_short_v2
 ; CHECK-NEXT: .hidden __hwasan_check_x0_2_short_v2
 ; CHECK-NEXT: __hwasan_check_x0_2_short_v2:
-; CHECK-NEXT: ubfx x16, x0, #4, #52
+; CHECK-NEXT: sbfx x16, x0, #4, #52
 ; CHECK-NEXT: ldrb w16, [x20, x16]
 ; CHECK-NEXT: cmp x16, x0, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp0
@@ -69,7 +69,7 @@
 ; CHECK-NEXT: .weak __hwasan_check_x1_1
 ; CHECK-NEXT: .hidden __hwasan_check_x1_1
 ; CHECK-NEXT: __hwasan_check_x1_1:
-; CHECK-NEXT: ubfx x16, x1, #4, #52
+; CHECK-NEXT: sbfx x16, x1, #4, #52
 ; CHECK-NEXT: ldrb w16, [x9, x16]
 ; CHECK-NEXT: cmp x16, x1, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp3
Index: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
===
--- llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -348,7 +348,7 @@
 OutStreamer->emitSymbolAttribute(Sym, MCSA_Hidden);
 OutStreamer->emitLabel(Sym);
 
-OutStreamer->emitInstruction(MCInstBuilder(AArch64::UBFMXri)
+OutStreamer->emitInstruction(MCInstBuilder(AArch64::SBFMXri)
  .addReg(AArch64::X16)
  .addReg(Reg)
  .addImm(4)
Index: clang/docs/HardwareAssistedAddressSanitizerDesign.rst
===
--- clang/docs/HardwareAssistedAddressSanitizerDesign.rst
+++ clang/docs/HardwareAssistedAddressSanitizerDesign.rst
@@ -96,7 +96,7 @@
 
   [...]
   __hwasan_check_x0_2_short_v2:
-ubfxx16, x0, #4, #52// shadow offset
+sbfxx16, x0, #4, #52// shadow offset
 ldrbw16, [x20, x16] // load shadow tag
 cmp x16, x0, lsr #56// extract address 
tag, compare with shadow tag
 b.ne.Ltmp0  // jump to short tag 
handler on mismatch


Index: llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
===
--- llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
+++ llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
@@ -38,7 +38,7 @@
 ; CHECK-NEXT: .weak __hwasan_check_x0_2_short_v2
 ; CHECK-NEXT: .hidden __hwasan_check_x0_2_short_v2
 ; CHECK-NEXT: __hwasan_check_x0_2_short_v2:
-; CHECK-NEXT: ubfx x16, x0, #4, #52
+; CHECK-NEXT: sbfx x16, x0, #4, #52
 ; CHECK-NEXT: ldrb w16, [x20, x16]
 ; CHECK-NEXT: cmp x16, x0, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp0
@@ -69,7 +69,7 @@
 ; CHECK-NEXT: .weak __hwasan_check_x1_1
 ; CHECK-NEXT: .hidden __hwasan_check_x1_1
 ; CHECK-NEXT: __hwasan_check_x1_1:
-; CHECK-NEXT: ubfx x16, x1, #4, #52
+; CHECK-NEXT: sbfx x16, x1, #4, #52
 ; CHECK-NEXT: ldrb w16, [x9, x16]
 ; CHECK-NEXT: cmp x16, x1, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp3
Index: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
===
--- llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -348,7 +348,7 @@
 OutStreamer->emitSymbolAttribute(Sym, MCSA_Hidden);
 OutStreamer->emitLabel(Sym);
 
-OutStreamer->emitInstruction(MCInstBuilder(AArch64::UBFMXri)
+OutStreamer->emitInstruction(MCInstBuilder(AArch64::SBFMXri)
  .addReg(AArch64::X16)
  .addReg(Reg)
  .addImm(4)
Index: clang/docs/HardwareAssistedAddressSanitizerDesign.rst
===
--- clang/docs/HardwareAssistedAddressSanitizerDesign.rst
+++ clang/docs/HardwareAssistedAddressSanitizerDesign.rst
@@ -96,7 +96,7 @@
 
   [...]
   __hwasan_check_x0_2_short_v2:
-ubfxx16, x0, #4, #52// shadow offset
+sbfxx16, x0, #4, #52// shadow offset
 ldrbw16, [x20, x16] // load shadow tag
 cmp x16, x0, lsr #56// extract address tag, compare with shadow tag
 b.ne.Ltmp0  

[PATCH] D90422: AArch64: Switch to x20 as the shadow base register for outlined HWASan checks.

[Peter Collingbourne 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 rG3859fc653fb4: AArch64: Switch to x20 as the shadow base 
register for outlined HWASan checks. (authored by pcc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90422

Files:
  clang/docs/HardwareAssistedAddressSanitizerDesign.rst
  compiler-rt/test/hwasan/TestCases/register-dump-read.c
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll

Index: llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
===
--- llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
+++ llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
@@ -18,12 +18,13 @@
 
 define i8* @f2(i8* %x0, i8* %x1) {
   ; CHECK: f2:
-  ; CHECK: str x30, [sp, #-16]!
+  ; CHECK: stp x30, x20, [sp, #-16]!
   ; CHECK-NEXT: .cfi_def_cfa_offset 16
+  ; CHECK-NEXT: .cfi_offset w20, -8
   ; CHECK-NEXT: .cfi_offset w30, -16
-  ; CHECK-NEXT: mov x9, x1
-  ; CHECK-NEXT: bl __hwasan_check_x0_2_short
-  ; CHECK-NEXT: ldr x30, [sp], #16
+  ; CHECK-NEXT: mov x20, x1
+  ; CHECK-NEXT: bl __hwasan_check_x0_2_short_v2
+  ; CHECK-NEXT: ldp x30, x20, [sp], #16
   ; CHECK-NEXT: ret
   call void @llvm.hwasan.check.memaccess.shortgranules(i8* %x1, i8* %x0, i32 2)
   ret i8* %x0
@@ -32,13 +33,13 @@
 declare void @llvm.hwasan.check.memaccess(i8*, i8*, i32)
 declare void @llvm.hwasan.check.memaccess.shortgranules(i8*, i8*, i32)
 
-; CHECK:  .section .text.hot,"axG",@progbits,__hwasan_check_x0_2_short,comdat
-; CHECK-NEXT: .type __hwasan_check_x0_2_short,@function
-; CHECK-NEXT: .weak __hwasan_check_x0_2_short
-; CHECK-NEXT: .hidden __hwasan_check_x0_2_short
-; CHECK-NEXT: __hwasan_check_x0_2_short:
+; CHECK:  .section .text.hot,"axG",@progbits,__hwasan_check_x0_2_short_v2,comdat
+; CHECK-NEXT: .type __hwasan_check_x0_2_short_v2,@function
+; CHECK-NEXT: .weak __hwasan_check_x0_2_short_v2
+; CHECK-NEXT: .hidden __hwasan_check_x0_2_short_v2
+; CHECK-NEXT: __hwasan_check_x0_2_short_v2:
 ; CHECK-NEXT: ubfx x16, x0, #4, #52
-; CHECK-NEXT: ldrb w16, [x9, x16]
+; CHECK-NEXT: ldrb w16, [x20, x16]
 ; CHECK-NEXT: cmp x16, x0, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp0
 ; CHECK-NEXT: .Ltmp1:
Index: llvm/lib/Target/AArch64/AArch64InstrInfo.td
===
--- llvm/lib/Target/AArch64/AArch64InstrInfo.td
+++ llvm/lib/Target/AArch64/AArch64InstrInfo.td
@@ -1123,9 +1123,12 @@
   (outs), (ins GPR64noip:$ptr, i32imm:$accessinfo),
   [(int_hwasan_check_memaccess X9, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
   Sched<[]>;
+}
+
+let Uses = [ X20 ], Defs = [ X16, X17, LR, NZCV ] in {
 def HWASAN_CHECK_MEMACCESS_SHORTGRANULES : Pseudo<
   (outs), (ins GPR64noip:$ptr, i32imm:$accessinfo),
-  [(int_hwasan_check_memaccess_shortgranules X9, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
+  [(int_hwasan_check_memaccess_shortgranules X20, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
   Sched<[]>;
 }
 
Index: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
===
--- llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -302,7 +302,7 @@
 std::string SymName = "__hwasan_check_x" + utostr(Reg - AArch64::X0) + "_" +
   utostr(AccessInfo);
 if (IsShort)
-  SymName += "_short";
+  SymName += "_short_v2";
 Sym = OutContext.getOrCreateSymbol(SymName);
   }
 
@@ -354,13 +354,14 @@
  .addImm(4)
  .addImm(55),
  *STI);
-OutStreamer->emitInstruction(MCInstBuilder(AArch64::LDRBBroX)
- .addReg(AArch64::W16)
- .addReg(AArch64::X9)
- .addReg(AArch64::X16)
- .addImm(0)
- .addImm(0),
- *STI);
+OutStreamer->emitInstruction(
+MCInstBuilder(AArch64::LDRBBroX)
+.addReg(AArch64::W16)
+.addReg(IsShort ? AArch64::X20 : AArch64::X9)
+.addReg(AArch64::X16)
+.addImm(0)
+.addImm(0),
+*STI);
 OutStreamer->emitInstruction(
 MCInstBuilder(AArch64::SUBSXrs)
 .addReg(AArch64::XZR)
Index: compiler-rt/test/hwasan/TestCases/register-dump-read.c
===
--- compiler-rt/test/hwasan/TestCases/register-dump-read.c
+++ compiler-rt/test/hwasan/TestCases/register-dump-read.c
@@ -15,7 +15,7 @@
   __hwasan_enable_allocator_tagging();
   char * volatile x = (char*) malloc(10);
   asm volatile("mov x10, 

[PATCH] D46791: Make -gsplit-dwarf generally available

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Correct, clang no longer uses objcopy for this as of D47093 
.


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

https://reviews.llvm.org/D46791

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

[PATCH] D90422: AArch64: Switch to x20 as the shadow base register for outlined HWASan checks.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

For the kernel I measured a small regression in boot time (with a version of 
this change that uses x20 for the v1 checks as well since the kernel doesn't 
use short granules yet) -- from 6.65s to 6.70s or 0.8%. But that's a fraction 
of the size gains which were 4% for kernel and (as mentioned) 3% for userspace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90422

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

[PATCH] D90424: AArch64: Use SBFX instead of UBFX to extract address granule in outlined HWASan checks.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: eugenis, hctim.
Herald added subscribers: cfe-commits, danielkiss, hiraditya, kristof.beyls.
Herald added projects: clang, LLVM.
pcc requested review of this revision.

In a kernel (or in general in environments where bit 55 of the address
is set) the shadow base needs to point to the end of the shadow region,
not the beginning. Bit 55 needs to be sign extended into bits 52-63
of the shadow base offset, otherwise we end up loading from an invalid
address. We can do this by using SBFX instead of UBFX.

Using SBFX should have no effect in the userspace case where bit 55
of the address is clear so we do so unconditionally. I don't think
we need a ABI version bump for this (but one will come anyway when
we switch to x20 for the shadow base register).

Depends on D90422 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90424

Files:
  clang/docs/HardwareAssistedAddressSanitizerDesign.rst
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll


Index: llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
===
--- llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
+++ llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
@@ -38,7 +38,7 @@
 ; CHECK-NEXT: .weak __hwasan_check_x0_2_short_v2
 ; CHECK-NEXT: .hidden __hwasan_check_x0_2_short_v2
 ; CHECK-NEXT: __hwasan_check_x0_2_short_v2:
-; CHECK-NEXT: ubfx x16, x0, #4, #52
+; CHECK-NEXT: sbfx x16, x0, #4, #52
 ; CHECK-NEXT: ldrb w16, [x20, x16]
 ; CHECK-NEXT: cmp x16, x0, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp0
@@ -69,7 +69,7 @@
 ; CHECK-NEXT: .weak __hwasan_check_x1_1
 ; CHECK-NEXT: .hidden __hwasan_check_x1_1
 ; CHECK-NEXT: __hwasan_check_x1_1:
-; CHECK-NEXT: ubfx x16, x1, #4, #52
+; CHECK-NEXT: sbfx x16, x1, #4, #52
 ; CHECK-NEXT: ldrb w16, [x9, x16]
 ; CHECK-NEXT: cmp x16, x1, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp3
Index: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
===
--- llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -348,7 +348,7 @@
 OutStreamer->emitSymbolAttribute(Sym, MCSA_Hidden);
 OutStreamer->emitLabel(Sym);
 
-OutStreamer->emitInstruction(MCInstBuilder(AArch64::UBFMXri)
+OutStreamer->emitInstruction(MCInstBuilder(AArch64::SBFMXri)
  .addReg(AArch64::X16)
  .addReg(Reg)
  .addImm(4)
Index: clang/docs/HardwareAssistedAddressSanitizerDesign.rst
===
--- clang/docs/HardwareAssistedAddressSanitizerDesign.rst
+++ clang/docs/HardwareAssistedAddressSanitizerDesign.rst
@@ -96,7 +96,7 @@
 
   [...]
   __hwasan_check_x0_2_short_v2:
-ubfxx16, x0, #4, #52// shadow offset
+sbfxx16, x0, #4, #52// shadow offset
 ldrbw16, [x20, x16] // load shadow tag
 cmp x16, x0, lsr #56// extract address 
tag, compare with shadow tag
 b.ne.Ltmp0  // jump to short tag 
handler on mismatch


Index: llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
===
--- llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
+++ llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
@@ -38,7 +38,7 @@
 ; CHECK-NEXT: .weak __hwasan_check_x0_2_short_v2
 ; CHECK-NEXT: .hidden __hwasan_check_x0_2_short_v2
 ; CHECK-NEXT: __hwasan_check_x0_2_short_v2:
-; CHECK-NEXT: ubfx x16, x0, #4, #52
+; CHECK-NEXT: sbfx x16, x0, #4, #52
 ; CHECK-NEXT: ldrb w16, [x20, x16]
 ; CHECK-NEXT: cmp x16, x0, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp0
@@ -69,7 +69,7 @@
 ; CHECK-NEXT: .weak __hwasan_check_x1_1
 ; CHECK-NEXT: .hidden __hwasan_check_x1_1
 ; CHECK-NEXT: __hwasan_check_x1_1:
-; CHECK-NEXT: ubfx x16, x1, #4, #52
+; CHECK-NEXT: sbfx x16, x1, #4, #52
 ; CHECK-NEXT: ldrb w16, [x9, x16]
 ; CHECK-NEXT: cmp x16, x1, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp3
Index: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
===
--- llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -348,7 +348,7 @@
 OutStreamer->emitSymbolAttribute(Sym, MCSA_Hidden);
 OutStreamer->emitLabel(Sym);
 
-OutStreamer->emitInstruction(MCInstBuilder(AArch64::UBFMXri)
+OutStreamer->emitInstruction(MCInstBuilder(AArch64::SBFMXri)
  .addReg(AArch64::X16)
  .addReg(Reg)
  .addImm(4)
Index: clang/docs/HardwareAssistedAddressSanitizerDesign.rst
===
--- 

[PATCH] D90422: AArch64: Switch to x20 as the shadow base register for outlined HWASan checks.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: eugenis, hctim.
Herald added subscribers: Sanitizers, cfe-commits, danielkiss, hiraditya, 
kristof.beyls.
Herald added projects: clang, Sanitizers, LLVM.
pcc requested review of this revision.

>From a code size perspective it turns out to be better to use a
callee-saved register to pass the shadow base. For non-leaf functions
it avoids the need to reload the shadow base into x9 after each
function call, at the cost of an additional stack slot to save the
caller's x20. But with x9 there is also a stack size cost, either
as a result of copying x9 to a callee-saved register across calls or
by spilling it to stack, so for the non-leaf functions the change to
stack usage is largely neutral.

It is also code size (and stack size) neutral for many leaf functions.
Although they now need to save/restore x20 this can typically be
combined via LDP/STP into the x30 save/restore. In the case where
the function needs callee-saved registers or stack spills we end up
needing, on average, 8 more bytes of stack and 1 more instruction
but given the improvements to other functions this seems like the
right tradeoff.

Since this is an ABI change for the outlined check functions they
have been renamed.

Unfortunately we cannot change the register for the v1 (non short
granules) check because the runtime assumes that the shadow base
register is stored in x9, so the v1 check still uses x9.

With this change code size of /system/lib64/*.so in an Android build
with HWASan goes from 200066976 bytes to 194085912 bytes, or a 3%
decrease.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90422

Files:
  clang/docs/HardwareAssistedAddressSanitizerDesign.rst
  compiler-rt/test/hwasan/TestCases/register-dump-read.c
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll

Index: llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
===
--- llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
+++ llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
@@ -18,12 +18,13 @@
 
 define i8* @f2(i8* %x0, i8* %x1) {
   ; CHECK: f2:
-  ; CHECK: str x30, [sp, #-16]!
+  ; CHECK: stp x30, x20, [sp, #-16]!
   ; CHECK-NEXT: .cfi_def_cfa_offset 16
+  ; CHECK-NEXT: .cfi_offset w20, -8
   ; CHECK-NEXT: .cfi_offset w30, -16
-  ; CHECK-NEXT: mov x9, x1
-  ; CHECK-NEXT: bl __hwasan_check_x0_2_short
-  ; CHECK-NEXT: ldr x30, [sp], #16
+  ; CHECK-NEXT: mov x20, x1
+  ; CHECK-NEXT: bl __hwasan_check_x0_2_short_v2
+  ; CHECK-NEXT: ldp x30, x20, [sp], #16
   ; CHECK-NEXT: ret
   call void @llvm.hwasan.check.memaccess.shortgranules(i8* %x1, i8* %x0, i32 2)
   ret i8* %x0
@@ -32,13 +33,13 @@
 declare void @llvm.hwasan.check.memaccess(i8*, i8*, i32)
 declare void @llvm.hwasan.check.memaccess.shortgranules(i8*, i8*, i32)
 
-; CHECK:  .section .text.hot,"axG",@progbits,__hwasan_check_x0_2_short,comdat
-; CHECK-NEXT: .type __hwasan_check_x0_2_short,@function
-; CHECK-NEXT: .weak __hwasan_check_x0_2_short
-; CHECK-NEXT: .hidden __hwasan_check_x0_2_short
-; CHECK-NEXT: __hwasan_check_x0_2_short:
+; CHECK:  .section .text.hot,"axG",@progbits,__hwasan_check_x0_2_short_v2,comdat
+; CHECK-NEXT: .type __hwasan_check_x0_2_short_v2,@function
+; CHECK-NEXT: .weak __hwasan_check_x0_2_short_v2
+; CHECK-NEXT: .hidden __hwasan_check_x0_2_short_v2
+; CHECK-NEXT: __hwasan_check_x0_2_short_v2:
 ; CHECK-NEXT: ubfx x16, x0, #4, #52
-; CHECK-NEXT: ldrb w16, [x9, x16]
+; CHECK-NEXT: ldrb w16, [x20, x16]
 ; CHECK-NEXT: cmp x16, x0, lsr #56
 ; CHECK-NEXT: b.ne .Ltmp0
 ; CHECK-NEXT: .Ltmp1:
Index: llvm/lib/Target/AArch64/AArch64InstrInfo.td
===
--- llvm/lib/Target/AArch64/AArch64InstrInfo.td
+++ llvm/lib/Target/AArch64/AArch64InstrInfo.td
@@ -1123,9 +1123,12 @@
   (outs), (ins GPR64noip:$ptr, i32imm:$accessinfo),
   [(int_hwasan_check_memaccess X9, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
   Sched<[]>;
+}
+
+let Uses = [ X20 ], Defs = [ X16, X17, LR, NZCV ] in {
 def HWASAN_CHECK_MEMACCESS_SHORTGRANULES : Pseudo<
   (outs), (ins GPR64noip:$ptr, i32imm:$accessinfo),
-  [(int_hwasan_check_memaccess_shortgranules X9, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
+  [(int_hwasan_check_memaccess_shortgranules X20, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
   Sched<[]>;
 }
 
Index: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
===
--- llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -302,7 +302,7 @@
 std::string SymName = "__hwasan_check_x" + utostr(Reg - AArch64::X0) + "_" +
   utostr(AccessInfo);
 if (IsShort)
-  SymName += "_short";
+  SymName += "_short_v2";
 Sym = OutContext.getOrCreateSymbol(SymName);
   }
 
@@ -354,13 +354,14 @@

[PATCH] D89766: Driver: Add integer sanitizers to trapping group automatically.

[Peter Collingbourne 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 rGc5acd3490b79: Driver: Add integer sanitizers to trapping 
group automatically. (authored by pcc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89766

Files:
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -786,6 +786,13 @@
 // CHECK-UBSAN-MINIMAL: 
"-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
 // CHECK-UBSAN-MINIMAL: "-fsanitize-minimal-runtime"
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer 
-fsanitize-trap=integer %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-INTSAN-TRAP
+// CHECK-INTSAN-TRAP: 
"-fsanitize-trap=integer-divide-by-zero,shift-base,shift-exponent,signed-integer-overflow,unsigned-integer-overflow,unsigned-shift-base,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change"
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer 
-fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-INTSAN-MINIMAL
+// CHECK-INTSAN-MINIMAL: 
"-fsanitize=integer-divide-by-zero,shift-base,shift-exponent,signed-integer-overflow,unsigned-integer-overflow,unsigned-shift-base,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change"
+// CHECK-INTSAN-MINIMAL: "-fsanitize-minimal-runtime"
+
 // RUN: %clang -target aarch64-linux-android -march=armv8-a+memtag 
-fsanitize=memtag -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-MEMTAG-MINIMAL
 // CHECK-MEMTAG-MINIMAL: "-fsanitize=memtag"
 // CHECK-MEMTAG-MINIMAL: "-fsanitize-minimal-runtime"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -60,8 +60,7 @@
 SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress;
 static const SanitizerMask NeedsLTO = SanitizerKind::CFI;
 static const SanitizerMask TrappingSupported =
-(SanitizerKind::Undefined & ~SanitizerKind::Vptr) |
-SanitizerKind::UnsignedIntegerOverflow | SanitizerKind::ImplicitConversion 
|
+(SanitizerKind::Undefined & ~SanitizerKind::Vptr) | SanitizerKind::Integer 
|
 SanitizerKind::Nullability | SanitizerKind::LocalBounds |
 SanitizerKind::CFI | SanitizerKind::FloatDivideByZero |
 SanitizerKind::ObjCCast;


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -786,6 +786,13 @@
 // CHECK-UBSAN-MINIMAL: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
 // CHECK-UBSAN-MINIMAL: "-fsanitize-minimal-runtime"
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer -fsanitize-trap=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTSAN-TRAP
+// CHECK-INTSAN-TRAP: "-fsanitize-trap=integer-divide-by-zero,shift-base,shift-exponent,signed-integer-overflow,unsigned-integer-overflow,unsigned-shift-base,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change"
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTSAN-MINIMAL
+// CHECK-INTSAN-MINIMAL: "-fsanitize=integer-divide-by-zero,shift-base,shift-exponent,signed-integer-overflow,unsigned-integer-overflow,unsigned-shift-base,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change"
+// CHECK-INTSAN-MINIMAL: "-fsanitize-minimal-runtime"
+
 // RUN: %clang -target aarch64-linux-android -march=armv8-a+memtag -fsanitize=memtag -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MEMTAG-MINIMAL
 // CHECK-MEMTAG-MINIMAL: "-fsanitize=memtag"
 // CHECK-MEMTAG-MINIMAL: "-fsanitize-minimal-runtime"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -60,8 +60,7 @@
 SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress;
 static const SanitizerMask NeedsLTO = SanitizerKind::CFI;
 static const SanitizerMask TrappingSupported =
-

[PATCH] D89766: Driver: Add integer sanitizers to trapping group automatically.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: jfb, eugenis.
Herald added a project: clang.
pcc requested review of this revision.

In D86000  we added a new sanitizer to the 
integer group
without adding it to the trapping group. This broke usage of
-fsanitize=integer -fsanitize-trap=integer or -fsanitize=integer
-fsanitize-minimal-runtime.

I think we can reasonably expect any new integer sanitizers to be
compatible with trapping and the minimal runtime, so add them to the
trapping group automatically.

Also add a test to ensure that any future additions of sanitizers
to the integer group will most likely result in test failures which
would lead to updates to the minimal runtime if necessary. For this
particular sanitizer no updates are required because it uses the
existing shift_out_of_bounds callback function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89766

Files:
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -786,6 +786,13 @@
 // CHECK-UBSAN-MINIMAL: 
"-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
 // CHECK-UBSAN-MINIMAL: "-fsanitize-minimal-runtime"
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer 
-fsanitize-trap=integer %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-INTSAN-TRAP
+// CHECK-INTSAN-TRAP: 
"-fsanitize-trap=integer-divide-by-zero,shift-base,shift-exponent,signed-integer-overflow,unsigned-integer-overflow,unsigned-shift-base,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change"
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer 
-fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-INTSAN-MINIMAL
+// CHECK-INTSAN-MINIMAL: 
"-fsanitize=integer-divide-by-zero,shift-base,shift-exponent,signed-integer-overflow,unsigned-integer-overflow,unsigned-shift-base,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change"
+// CHECK-INTSAN-MINIMAL: "-fsanitize-minimal-runtime"
+
 // RUN: %clang -target aarch64-linux-android -march=armv8-a+memtag 
-fsanitize=memtag -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-MEMTAG-MINIMAL
 // CHECK-MEMTAG-MINIMAL: "-fsanitize=memtag"
 // CHECK-MEMTAG-MINIMAL: "-fsanitize-minimal-runtime"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -60,8 +60,7 @@
 SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress;
 static const SanitizerMask NeedsLTO = SanitizerKind::CFI;
 static const SanitizerMask TrappingSupported =
-(SanitizerKind::Undefined & ~SanitizerKind::Vptr) |
-SanitizerKind::UnsignedIntegerOverflow | SanitizerKind::ImplicitConversion 
|
+(SanitizerKind::Undefined & ~SanitizerKind::Vptr) | SanitizerKind::Integer 
|
 SanitizerKind::Nullability | SanitizerKind::LocalBounds |
 SanitizerKind::CFI | SanitizerKind::FloatDivideByZero |
 SanitizerKind::ObjCCast;


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -786,6 +786,13 @@
 // CHECK-UBSAN-MINIMAL: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
 // CHECK-UBSAN-MINIMAL: "-fsanitize-minimal-runtime"
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer -fsanitize-trap=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTSAN-TRAP
+// CHECK-INTSAN-TRAP: "-fsanitize-trap=integer-divide-by-zero,shift-base,shift-exponent,signed-integer-overflow,unsigned-integer-overflow,unsigned-shift-base,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change"
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTSAN-MINIMAL
+// CHECK-INTSAN-MINIMAL: "-fsanitize=integer-divide-by-zero,shift-base,shift-exponent,signed-integer-overflow,unsigned-integer-overflow,unsigned-shift-base,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change"
+// CHECK-INTSAN-MINIMAL: "-fsanitize-minimal-runtime"
+
 // RUN: %clang -target aarch64-linux-android -march=armv8-a+memtag -fsanitize=memtag -fsanitize-minimal-runtime %s -### 2>&1 | 

[PATCH] D87717: [docs] Update ControlFlowIntegrity.rst.

[Peter Collingbourne via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87717

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

[PATCH] D87095: [Triple][MachO] Define "arm64e", an AArch64 subarch for Pointer Auth.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Hi, thanks for getting started on upstreaming this!

But I was wondering: shouldn't this be the *last* patch? I was imagining that 
you would first upstream support for the `-fptrauth-*` flags, and then at the 
end you would add an `arm64e` subarch that turns them on by default. Otherwise 
the upstream compiler will temporarily produce ABI-incompatible objects when 
targeting `arm64e`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87095

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