[PATCH] D152604: [Driver] Default -fsanitize-address-globals-dead-stripping to true for ELF

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM

This is long overdue, I think. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152604

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I'm fine with it in general. Is asan_abi.cpp meant as a temporary stub? It's 
not even link anywhere in the current version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D148596: [KMSAN] Enable on SystemZ

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D148596/new/

https://reviews.llvm.org/D148596

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

[PATCH] D147121: [hwasan] remove requirment for PIE

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Sorry, I do not remember why this requirement is there. Must be related to 
shadow / allocator placement and kernel mapping conflicts, but hwasan is using 
dynamic shadow so that should not be an issue... LGTM as long as it works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147121

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

[PATCH] D133392: [MTE] Add AArch64GlobalsTagging Pass

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM




Comment at: llvm/lib/CodeGen/GlobalMerge.cpp:655
 
+// Don't merged tagged globals, as each global should have its own unique
+// memory tag at runtime. TODO(hctim): This can be relaxed: constant 
globals

typo: "merge"



Comment at: llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp:75
+  if (SizeInBytes != NewSize) {
+DataLayout NewLayout = M.getDataLayout();
+

unused variable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133392

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

[PATCH] D128958: Add assembler plumbing for sanitize_memtag

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:713
 
+  if (GV->hasSanitizerMetadata() && GV->isTagged()) {
+Triple T = TM.getTargetTriple();

isTagged() already includes a check for hasSanitizerMetadata()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128958

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

[PATCH] D133392: [MTE] Add AArch64GlobalsTagging Pass

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Change description says that the new pass "marks them as tagged". That's not 
what is happening.




Comment at: llvm/lib/CodeGen/GlobalMerge.cpp:657
+// Tagged global variables shouldn't be merged, as they are assigned unique
+// memory tags at runtime.
+if (GV.isTagged())

This comment is not convincing. The change description is right that some 
globals can be merged, copy that here, or extend the comment a little to 
explain why and under what assumptions.



Comment at: llvm/lib/MC/ELFObjectWriter.cpp:1344
+  // when relocating `end` symbols, and this can only be determined by the
+  // attributes of the symbol itself.
+  if (Sym->isMemtag())

Isn't a bigger reason that we need to know the address tag to put on the 
pointer, and that requires a symbol, not a section reference?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133392

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

[PATCH] D131438: [clang][dataflow] Analyze constructor bodies

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Sorry but I had to revert this because of the conflicts with another revert: 
https://reviews.llvm.org/D131065


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131438

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

[PATCH] D131065: [clang][dataflow] Store DeclContext of block being analysed in Environment if available.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Reverted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131065

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

[PATCH] D131065: [clang][dataflow] Store DeclContext of block being analysed in Environment if available.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

MSan is not happy:
https://lab.llvm.org/buildbot/#/builders/74/builds/12717


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131065

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

[PATCH] D129492: Add missing sanitizer metadata plumbing from CFE.

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM

Is there an ASan test to update? I guess before this change, for an 
ignorelisted global, ASan would emit wrongly typed declaration on the undef 
side - which should not cause any practical issues, but still.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129492

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

[PATCH] D128950: Remove 'no_sanitize_memtag'. Add 'sanitize_memtag'.

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM




Comment at: llvm/include/llvm/IR/GlobalValue.h:349
+const SanitizerMetadata& Meta = getSanitizerMetadata();
+return Meta.Memtag;
+  }

`return hasSanitizerMetadata() && getSanitizerMetadata().Memtag`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128950

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

[PATCH] D127860: [msan] Allow KMSAN to use -fsanitize-memory-param-retval

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D127860/new/

https://reviews.llvm.org/D127860

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

[PATCH] D127163: [clang] Add -fsanitize=memtag-globals (no-op).

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D127163/new/

https://reviews.llvm.org/D127163

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

[PATCH] D127163: [clang] Add -fsanitize=memtag-globals (no-op).

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I don't have a lot of arguments either way, do you?
Ignorelist support is one for the new sanitizer type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127163

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

[PATCH] D127163: [clang] Add -fsanitize=memtag-globals (no-op).

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:767
+  if (IsAArch64) {
+Res |= SanitizerKind::MemtagGlobals;
+  }

Hmm why are all the other memtag* not here?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7892
 
+static bool isSanitizerThatUsesGlobals(StringRef Sanitizer) {
+  return Sanitizer == "address" || Sanitizer == "hwaddress" ||

Maybe "applies to globals"? On the other hand, MSan "applies" to globals but 
does not need this logic.

isSanitizerAttributeAllowedOnGlobals?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127163

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

[PATCH] D127145: [Driver] add -lresolv for all but Android.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

Typo: "runttime"

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127145

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

[PATCH] D127145: [Driver] add -lresolv for all but Android.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

The interceptor (dn_expand) is only defined on linux && !android, while this 
code links libresolv on *bsd and others, too. I think it's ok, as long as all 
platforms with the interceptor are covered.

It would be great to mention the actual reason we do this in the commit 
message: because otherwise link tests in ./configure scripts may be confused by 
the presence of the dn_expand symbol even without -lresolv (because of the 
interceptor) and get a runtime error later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127145

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

[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I've always viewed the current implementation as a hack, and believed this data 
should live in debug info.

It can be convenient to get symbolized reports for global overflow bug in a 
stripped binary, but those bugs are quite rare, and "symbolization" only 
affects the global itself, not the stack trace of the bad memory access. It 
does not seem worth it to support two code paths just for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493

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

[PATCH] D118948: [MTE] Add -fsanitize=memtag* and friends.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM




Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:475
 def err_stack_tagging_requires_hardware_feature : Error<
-  "'-fsanitize=memtag' requires hardware support (+memtag)">;
+  "'-fsanitize=memtag-stack' requires hardware support (+memtag). For Armv8, "
+  "try compiling with -march=armv8a+memtag.">;

MaskRay wrote:
> hctim wrote:
> > eugenis wrote:
> > > Split out renaming of memtag to memtag-stack first? This will remove a 
> > > lot of diff from this patch.
> > splitting into elf -> lld -> clang as per Ray's suggestion, should reduce 
> > the diff enough.
> The convention is to omit period for the last sentence.
Mention armv9a, too?
Something like "Try compiling with -march=armv9a, or -march=armv8a+memtag"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118948

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

[PATCH] D122685: [msan] Add link to the lifetime definition

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D122685/new/

https://reviews.llvm.org/D122685

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

[PATCH] D120437: [HWASAN] erase lifetime intrinsics if tag is outside.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/test/CodeGen/lifetime-sanitizer.c:9
+// RUN: %clang -target aarch64-linux-gnu -S -emit-llvm -o /dev/null -O0 \
+// RUN: -fsanitize=hwaddress -mllvm -print-before=hwasan %s 2>&1 | \
 // RUN: FileCheck %s -check-prefix=LIFETIME

fmayer wrote:
> eugenis wrote:
> > You can use -Xclang -disable-llvm-passes instead.
> Isn't what is currently there closer to what we actually want to test: that 
> the hwasan pass has access to lifetimes?
I don't have a strong opinion on this, but it is common to exclude llvm passes 
from clang tests because that makes them brittle. Integration tests generally 
belong in compiler-rt, but there are some in clang, too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120437

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

[PATCH] D120437: [HWASAN] erase lifetime intrinsics if tag is outside.

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/test/CodeGen/lifetime-sanitizer.c:9
+// RUN: %clang -target aarch64-linux-gnu -S -emit-llvm -o /dev/null -O0 \
+// RUN: -fsanitize=hwaddress -mllvm -print-before=hwasan %s 2>&1 | \
 // RUN: FileCheck %s -check-prefix=LIFETIME

You can use -Xclang -disable-llvm-passes instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120437

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

[PATCH] D119726: [asan] Add support for disable_sanitizer_instrumentation attribute

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119726

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

[PATCH] D119600: Stricter use-after-dtor detection for trivial members.

[Evgenii Stepanov via Phabricator via cfe-commits]

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

Poison trivial class members one-by-one in the reverse order of their
construction, instead of all-at-once at the very end.

For example, in the following code access to `x` from `~B` will
produce an undefined value.

struct A {

  struct B b;
  int x;

};


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119600

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/sanitize-dtor-nontrivial-virtual-base.cpp
  clang/test/CodeGenCXX/sanitize-dtor-zero-size-field.cpp

Index: clang/test/CodeGenCXX/sanitize-dtor-zero-size-field.cpp
===
--- clang/test/CodeGenCXX/sanitize-dtor-zero-size-field.cpp
+++ clang/test/CodeGenCXX/sanitize-dtor-zero-size-field.cpp
@@ -44,7 +44,9 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T1
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T16StructD2Ev(
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 13)
+// CHECK: [[GEP:%.+]] = getelementptr i8, {{.*}}, i64 8{{$}}
+// CHECK: call void @__sanitizer_dtor_callback(i8* [[GEP]], i64 13)
+// CHECK: call void @_ZN10NonTrivialD1Ev(
 // CHECK-NEXT:ret void
 
 namespace T2 {
@@ -57,8 +59,11 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T2
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T26StructD2Ev(
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 8)
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 5)
+// CHECK: [[GEP1:%.+]] = getelementptr i8, {{.*}}, i64 16{{$}}
+// CHECK: call void @__sanitizer_dtor_callback(i8* [[GEP1]], i64 5)
+// CHECK: call void @_ZN10NonTrivialD1Ev(
+// CHECK: [[GEP2:%.+]] = getelementptr i8, {{.*}}, i64 0{{$}}
+// CHECK: call void @__sanitizer_dtor_callback(i8* [[GEP2]], i64 8)
 // CHECK-NEXT:ret void
 
 namespace T3 {
@@ -71,8 +76,11 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T3
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T36StructD2Ev(
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 12)
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 1)
+// CHECK: [[GEP1:%.+]] = getelementptr i8, {{.*}}, i64 20{{$}}
+// CHECK: call void @__sanitizer_dtor_callback(i8* [[GEP1]], i64 1)
+// CHECK: call void @_ZN10NonTrivialD1Ev(
+// CHECK: [[GEP2:%.+]] = getelementptr i8, {{.*}}, i64 0{{$}}
+// CHECK: call void @__sanitizer_dtor_callback(i8* [[GEP2]], i64 12)
 // CHECK-NEXT:ret void
 
 namespace T4 {
@@ -100,6 +108,7 @@
 } // namespace T5
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T56StructD2Ev(
 // CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 13)
+// CHECK: call void @_ZN10NonTrivialD1Ev(
 // CHECK-NEXT:ret void
 
 namespace T6 {
@@ -114,6 +123,7 @@
 } // namespace T6
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T66StructD2Ev(
 // CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 13)
+// CHECK: call void @_ZN10NonTrivialD1Ev(
 // CHECK-NEXT:ret void
 
 namespace T7 {
@@ -127,8 +137,9 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T7
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T76StructD2Ev(
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 8)
 // CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 5)
+// CHECK: call void @_ZN10NonTrivialD1Ev(
+// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 8)
 // CHECK-NEXT:ret void
 
 namespace T8 {
@@ -142,8 +153,8 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T8
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T86StructD2Ev(
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 8)
 // CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 5)
+// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 8)
 // CHECK-NEXT:ret void
 
 namespace T9 {
@@ -157,8 +168,8 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T9
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T96StructD2Ev(
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 12)
 // CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 1)
+// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 12)
 // CHECK-NEXT:ret void
 
 namespace T10 {
@@ -172,8 +183,8 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T10
 // CHECK-LABEL: define {{.*}} @_ZN5empty3T106StructD2Ev(
-// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 12)
 // CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 1)
+// CHECK: call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 12)
 // CHECK-NEXT:ret void
 
 namespace T11 {
@@ -202,6 +213,7 @@
 } // namespace T12
 } // namespace empty
 // CHECK-LABEL: 

[PATCH] D119300: Use-after-dtor detection for trivial base classes.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis updated this revision to Diff 408071.
eugenis added a comment.

Fix handling of empty base classes and suppress tail calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119300

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/sanitize-dtor-trivial-base.cpp
  compiler-rt/test/msan/dtor-base-access.cpp

Index: compiler-rt/test/msan/dtor-base-access.cpp
===
--- compiler-rt/test/msan/dtor-base-access.cpp
+++ compiler-rt/test/msan/dtor-base-access.cpp
@@ -9,41 +9,62 @@
 
 class Base {
  public:
-  int *x_ptr;
-  Base(int *y_ptr) {
-// store value of subclass member
-x_ptr = y_ptr;
-  }
-  virtual ~Base();
+   int b;
+   Base() { b = 1; }
+   ~Base();
 };
 
-class Derived : public Base {
- public:
-  int y;
-  Derived():Base() {
-y = 10;
-  }
+class TrivialBaseBefore {
+public:
+  int tb0;
+  TrivialBaseBefore() { tb0 = 1; }
+};
+
+class TrivialBaseAfter {
+public:
+  int tb1;
+  TrivialBaseAfter() { tb1 = 1; }
+};
+
+class Derived : public TrivialBaseBefore, public Base, public TrivialBaseAfter {
+public:
+  int d;
+  Derived() { d = 1; }
   ~Derived();
 };
 
+Derived *g;
+
 Base::~Base() {
-  // ok access its own member
-  assert(__msan_test_shadow(>x_ptr, sizeof(this->x_ptr)) == -1);
-  // bad access subclass member
-  assert(__msan_test_shadow(this->x_ptr, sizeof(*this->x_ptr)) != -1);
+  // ok to access its own members and earlier bases
+  assert(__msan_test_shadow(>tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(>b, sizeof(g->b)) == -1);
+  // not ok to access others
+  assert(__msan_test_shadow(>tb1, sizeof(g->tb1)) == 0);
+  assert(__msan_test_shadow(>d, sizeof(g->d)) == 0);
 }
 
 Derived::~Derived() {
-  // ok to access its own members
-  assert(__msan_test_shadow(>y, sizeof(this->y)) == -1);
-  // ok access base class members
-  assert(__msan_test_shadow(>x_ptr, sizeof(this->x_ptr)) == -1);
+  // ok to access everything
+  assert(__msan_test_shadow(>tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(>b, sizeof(g->b)) == -1);
+  assert(__msan_test_shadow(>tb1, sizeof(g->tb1)) == -1);
+  assert(__msan_test_shadow(>d, sizeof(g->d)) == -1);
 }
 
 int main() {
-  Derived *d = new Derived();
-  assert(__msan_test_shadow(>x_ptr, sizeof(d->x_ptr)) == -1);
-  d->~Derived();
-  assert(__msan_test_shadow(>x_ptr, sizeof(d->x_ptr)) != -1);
+  g = new Derived();
+  // ok to access everything
+  assert(__msan_test_shadow(>tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(>b, sizeof(g->b)) == -1);
+  assert(__msan_test_shadow(>tb1, sizeof(g->tb1)) == -1);
+  assert(__msan_test_shadow(>d, sizeof(g->d)) == -1);
+
+  g->~Derived();
+  // not ok to access everything
+  assert(__msan_test_shadow(>tb0, sizeof(g->tb0)) == 0);
+  assert(__msan_test_shadow(>b, sizeof(g->b)) == 0);
+  assert(__msan_test_shadow(>tb1, sizeof(g->tb1)) == 0);
+  assert(__msan_test_shadow(>d, sizeof(g->d)) == 0);
   return 0;
 }
Index: clang/test/CodeGenCXX/sanitize-dtor-trivial-base.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/sanitize-dtor-trivial-base.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-passes -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -O1 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-passes -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+
+// Base class has trivial dtor => complete dtor poisons base class memory directly.
+
+class Base {
+ public:
+  int x[4];
+};
+
+class Derived : public Base {
+ public:
+  int y;
+  ~Derived() {
+  }
+};
+
+Derived d;
+
+// Poison members, then poison the trivial base class.
+// CHECK-LABEL: define {{.*}}DerivedD2Ev
+// CHECK: %[[GEP:[0-9a-z]+]] = getelementptr i8, i8* {{.*}}, i64 16
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}%[[GEP]], i64 4
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}, i64 16
+// CHECK: ret void
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -1666,6 +1666,34 @@
 CGF.EmitNounwindRuntimeCall(Fn, Args);
   }
 
+  /// Poison base class with a trivial destructor.
+  struct SanitizeDtorTrivialBase final : EHScopeStack::Cleanup {
+const CXXRecordDecl *BaseClass;
+bool BaseIsVirtual;
+SanitizeDtorTrivialBase(const CXXRecordDecl *Base, bool BaseIsVirtual)
+: BaseClass(Base), BaseIsVirtual(BaseIsVirtual) {}
+
+void Emit(CodeGenFunction , Flags flags) override {
+  const CXXRecordDecl *DerivedClass =
+  cast(CGF.CurCodeDecl)->getParent();
+
+  Address Addr = CGF.GetAddressOfDirectBaseInCompleteClass(
+  CGF.LoadCXXThisAddress(), DerivedClass, BaseClass, BaseIsVirtual);
+
+  

[PATCH] D119299: [NFC] clang-format one function.

[Evgenii Stepanov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa730b6a41ad7: [NFC] clang-format one function. (authored by 
eugenis).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119299

Files:
  clang/lib/CodeGen/CGClass.cpp


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -1649,22 +1649,22 @@
 }
   };
 
- static void EmitSanitizerDtorCallback(CodeGenFunction , llvm::Value *Ptr,
- CharUnits::QuantityType PoisonSize) {
-   CodeGenFunction::SanitizerScope SanScope();
-   // Pass in void pointer and size of region as arguments to runtime
-   // function
-   llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, CGF.VoidPtrTy),
-  llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
-
-   llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
-
-   llvm::FunctionType *FnType =
-   llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
-   llvm::FunctionCallee Fn =
-   CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
-   CGF.EmitNounwindRuntimeCall(Fn, Args);
- }
+  static void EmitSanitizerDtorCallback(CodeGenFunction , llvm::Value *Ptr,
+CharUnits::QuantityType PoisonSize) {
+CodeGenFunction::SanitizerScope SanScope();
+// Pass in void pointer and size of region as arguments to runtime
+// function
+llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, CGF.VoidPtrTy),
+   llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
+
+llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
+
+llvm::FunctionType *FnType =
+llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
+llvm::FunctionCallee Fn =
+CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
+CGF.EmitNounwindRuntimeCall(Fn, Args);
+  }
 
   class SanitizeDtorMembers final : public EHScopeStack::Cleanup {
 const CXXDestructorDecl *Dtor;


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -1649,22 +1649,22 @@
 }
   };
 
- static void EmitSanitizerDtorCallback(CodeGenFunction , llvm::Value *Ptr,
- CharUnits::QuantityType PoisonSize) {
-   CodeGenFunction::SanitizerScope SanScope();
-   // Pass in void pointer and size of region as arguments to runtime
-   // function
-   llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, CGF.VoidPtrTy),
-  llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
-
-   llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
-
-   llvm::FunctionType *FnType =
-   llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
-   llvm::FunctionCallee Fn =
-   CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
-   CGF.EmitNounwindRuntimeCall(Fn, Args);
- }
+  static void EmitSanitizerDtorCallback(CodeGenFunction , llvm::Value *Ptr,
+CharUnits::QuantityType PoisonSize) {
+CodeGenFunction::SanitizerScope SanScope();
+// Pass in void pointer and size of region as arguments to runtime
+// function
+llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, CGF.VoidPtrTy),
+   llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
+
+llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
+
+llvm::FunctionType *FnType =
+llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
+llvm::FunctionCallee Fn =
+CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
+CGF.EmitNounwindRuntimeCall(Fn, Args);
+  }
 
   class SanitizeDtorMembers final : public EHScopeStack::Cleanup {
 const CXXDestructorDecl *Dtor;
___
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.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

You might want to update Bitcode writer/reader as well, otherwise nosanitize 
will be lost after a trip through .bc/.ii.




Comment at: compiler-rt/test/hwasan/TestCases/global-with-reduction.c:50
+  f()[atoi(argv[1])] = 1;
+  f()[atoi(argv[1])] = 1;
+  return 0;

did you mean to do it twice?


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] D119300: Use-after-dtor detection for trivial base classes.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis created this revision.
eugenis added reviewers: kda, vitalybuka.
eugenis requested review of this revision.
Herald added projects: clang, Sanitizers.
Herald added a subscriber: Sanitizers.

-fsanitize-memory-use-after-dtor detects memory access after a
subobject is destroyed but its memory is not yet deallocated.
This is done by poisoning each object memory near the end of its destructor.

Subobjects (members and base classes) do this in their respective
destructors, and the parent class does the same for its members with
trivial destructors.

Inexplicably, base classes with trivial destructors are not handled at
all. This change fixes this oversight by adding the base class poisoning logic
to the parent class destructor.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119300

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/sanitize-dtor-trivial-base.cpp
  compiler-rt/test/msan/dtor-base-access.cpp

Index: compiler-rt/test/msan/dtor-base-access.cpp
===
--- compiler-rt/test/msan/dtor-base-access.cpp
+++ compiler-rt/test/msan/dtor-base-access.cpp
@@ -9,41 +9,62 @@
 
 class Base {
  public:
-  int *x_ptr;
-  Base(int *y_ptr) {
-// store value of subclass member
-x_ptr = y_ptr;
-  }
-  virtual ~Base();
+   int b;
+   Base() { b = 1; }
+   ~Base();
 };
 
-class Derived : public Base {
- public:
-  int y;
-  Derived():Base() {
-y = 10;
-  }
+class TrivialBaseBefore {
+public:
+  int tb0;
+  TrivialBaseBefore() { tb0 = 1; }
+};
+
+class TrivialBaseAfter {
+public:
+  int tb1;
+  TrivialBaseAfter() { tb1 = 1; }
+};
+
+class Derived : public TrivialBaseBefore, public Base, public TrivialBaseAfter {
+public:
+  int d;
+  Derived() { d = 1; }
   ~Derived();
 };
 
+Derived *g;
+
 Base::~Base() {
-  // ok access its own member
-  assert(__msan_test_shadow(>x_ptr, sizeof(this->x_ptr)) == -1);
-  // bad access subclass member
-  assert(__msan_test_shadow(this->x_ptr, sizeof(*this->x_ptr)) != -1);
+  // ok to access its own members and earlier bases
+  assert(__msan_test_shadow(>tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(>b, sizeof(g->b)) == -1);
+  // not ok to access others
+  assert(__msan_test_shadow(>tb1, sizeof(g->tb1)) == 0);
+  assert(__msan_test_shadow(>d, sizeof(g->d)) == 0);
 }
 
 Derived::~Derived() {
-  // ok to access its own members
-  assert(__msan_test_shadow(>y, sizeof(this->y)) == -1);
-  // ok access base class members
-  assert(__msan_test_shadow(>x_ptr, sizeof(this->x_ptr)) == -1);
+  // ok to access everything
+  assert(__msan_test_shadow(>tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(>b, sizeof(g->b)) == -1);
+  assert(__msan_test_shadow(>tb1, sizeof(g->tb1)) == -1);
+  assert(__msan_test_shadow(>d, sizeof(g->d)) == -1);
 }
 
 int main() {
-  Derived *d = new Derived();
-  assert(__msan_test_shadow(>x_ptr, sizeof(d->x_ptr)) == -1);
-  d->~Derived();
-  assert(__msan_test_shadow(>x_ptr, sizeof(d->x_ptr)) != -1);
+  g = new Derived();
+  // ok to access everything
+  assert(__msan_test_shadow(>tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(>b, sizeof(g->b)) == -1);
+  assert(__msan_test_shadow(>tb1, sizeof(g->tb1)) == -1);
+  assert(__msan_test_shadow(>d, sizeof(g->d)) == -1);
+
+  g->~Derived();
+  // not ok to access everything
+  assert(__msan_test_shadow(>tb0, sizeof(g->tb0)) == 0);
+  assert(__msan_test_shadow(>b, sizeof(g->b)) == 0);
+  assert(__msan_test_shadow(>tb1, sizeof(g->tb1)) == 0);
+  assert(__msan_test_shadow(>d, sizeof(g->d)) == 0);
   return 0;
 }
Index: clang/test/CodeGenCXX/sanitize-dtor-trivial-base.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/sanitize-dtor-trivial-base.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-passes -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -O1 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-passes -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+
+// Base class has trivial dtor => complete dtor poisons base class memory directly.
+
+class Base {
+ public:
+  int x[4];
+};
+
+class Derived : public Base {
+ public:
+  int y;
+  ~Derived() {
+  }
+};
+
+Derived d;
+
+// Poison members, then poison the trivial base class.
+// CHECK-LABEL: define {{.*}}DerivedD2Ev
+// CHECK: %[[GEP:[0-9a-z]+]] = getelementptr i8, i8* {{.*}}, i64 16
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}%[[GEP]], i64 4
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}, i64 16
+// CHECK: ret void
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -1666,6 +1666,31 @@
 CGF.EmitNounwindRuntimeCall(Fn, Args);
   }
 
+  /// Poison base class with a trivial destructor.
+  struct 

[PATCH] D119299: [NFC] clang-format one function.

[Evgenii Stepanov via Phabricator via cfe-commits]

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

fix code formatting


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119299

Files:
  clang/lib/CodeGen/CGClass.cpp


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -1649,22 +1649,22 @@
 }
   };
 
- static void EmitSanitizerDtorCallback(CodeGenFunction , llvm::Value *Ptr,
- CharUnits::QuantityType PoisonSize) {
-   CodeGenFunction::SanitizerScope SanScope();
-   // Pass in void pointer and size of region as arguments to runtime
-   // function
-   llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, CGF.VoidPtrTy),
-  llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
-
-   llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
-
-   llvm::FunctionType *FnType =
-   llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
-   llvm::FunctionCallee Fn =
-   CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
-   CGF.EmitNounwindRuntimeCall(Fn, Args);
- }
+  static void EmitSanitizerDtorCallback(CodeGenFunction , llvm::Value *Ptr,
+CharUnits::QuantityType PoisonSize) {
+CodeGenFunction::SanitizerScope SanScope();
+// Pass in void pointer and size of region as arguments to runtime
+// function
+llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, CGF.VoidPtrTy),
+   llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
+
+llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
+
+llvm::FunctionType *FnType =
+llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
+llvm::FunctionCallee Fn =
+CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
+CGF.EmitNounwindRuntimeCall(Fn, Args);
+  }
 
   class SanitizeDtorMembers final : public EHScopeStack::Cleanup {
 const CXXDestructorDecl *Dtor;


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -1649,22 +1649,22 @@
 }
   };
 
- static void EmitSanitizerDtorCallback(CodeGenFunction , llvm::Value *Ptr,
- CharUnits::QuantityType PoisonSize) {
-   CodeGenFunction::SanitizerScope SanScope();
-   // Pass in void pointer and size of region as arguments to runtime
-   // function
-   llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, CGF.VoidPtrTy),
-  llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
-
-   llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
-
-   llvm::FunctionType *FnType =
-   llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
-   llvm::FunctionCallee Fn =
-   CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
-   CGF.EmitNounwindRuntimeCall(Fn, Args);
- }
+  static void EmitSanitizerDtorCallback(CodeGenFunction , llvm::Value *Ptr,
+CharUnits::QuantityType PoisonSize) {
+CodeGenFunction::SanitizerScope SanScope();
+// Pass in void pointer and size of region as arguments to runtime
+// function
+llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, CGF.VoidPtrTy),
+   llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
+
+llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
+
+llvm::FunctionType *FnType =
+llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
+llvm::FunctionCallee Fn =
+CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
+CGF.EmitNounwindRuntimeCall(Fn, Args);
+  }
 
   class SanitizeDtorMembers final : public EHScopeStack::Cleanup {
 const CXXDestructorDecl *Dtor;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118948: [MTE] Add -fsanitize=memtag* and friends.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:475
 def err_stack_tagging_requires_hardware_feature : Error<
-  "'-fsanitize=memtag' requires hardware support (+memtag)">;
+  "'-fsanitize=memtag-stack' requires hardware support (+memtag). For Armv8, "
+  "try compiling with -march=armv8a+memtag.">;

Split out renaming of memtag to memtag-stack first? This will remove a lot of 
diff from this patch.



Comment at: clang/include/clang/Driver/Options.td:1619
+Group,
+HelpText<"Set default MTE mode to 
'async' (default) or 'sync'.">;
 def fsanitize_hwaddress_experimental_aliasing

Let's make "sync" the default mode.



Comment at: lld/ELF/Config.h:346
+  // Mode of MTE to write to the ELF note. Should be one of NT_MEMTAG_ASYNC 
(for
+  // async), NT_MEMTAG_SYNC (for sync), or NT_MEMTAG_DEFAULT (for none). If
+  // async or sync is enabled, write the ELF note specifying the default MTE

NT_MEMTAG_DEFAULT -> NT_MEMTAG_LEVEL_NONE



Comment at: lld/ELF/Driver.cpp:691
+  error("When using --memtag-stack or --memtag-heap, a --memtag-mode value 
"
+"is required.");
+  } else {

Should it be required? We could assume default Sync here the same as in clang 
frontend, that's one less flag to pass around or list in manual linker 
invocations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118948

___
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.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

The driver seems to always add sanitizer runtimes first on the linker command 
line.




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

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.


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.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a subscriber: pcc.
eugenis added a comment.

LGTM, but I'd like @pcc to take a look.


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] D116633: Add -fsanitize-address-param-retval to clang.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2384
+if ((CodeGenOpts.EnableNoundefAttrs ||
+ CodeGenOpts.SanitizeMemoryParamRetval) &&
+ArgNoUndef)

vitalybuka wrote:
> vitalybuka wrote:
> > kda wrote:
> > > eugenis wrote:
> > > > vitalybuka wrote:
> > > > > vitalybuka wrote:
> > > > > > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if 
> > > > > > SanitizeMemoryParamRetval is provided?
> > > > > > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if 
> > > > > > SanitizeMemoryParamRetval is provided?
> > > > > 
> > > > > To clarify, checking SanitizeMemoryParamRetval here as-is is LGTM, 
> > > > > unless @eugenis or someone else thinks EnableNoundefAttrs reset is 
> > > > > better.
> > > > I don't think this is right at all. An argument being noundef has 
> > > > nothing to do at all with memory sanitization. It affects optimization, 
> > > > too. SanitizeMemoryParamRetval should only affect what 
> > > > MemorySanitizerPass does with noundef attributes.
> > > Is the problem the form of the new code?
> > >   - instead of introducing a single new flag to flip 4 others, should 
> > > there be 1 flag to pick up the CodeGen changes, and another for the 
> > > Sanitizer?  (Is 2 flags better than 4?)
> > >   - another option is have the changes propagate in the other direction: 
> > > use the flag (-fsanitize-memory-param-retval), to passes along '-mllvm 
> > > -enable_noundef_analysis=1' to CodeGen (via SanitizerArgs.addArgs).
> > > 
> > > OR is there a problem forcing EnableNoundefAttrs based on an "unrelated" 
> > > flag?
> > >   - then leave existing code, just don't do anything fancy to change 
> > > EnableNoundefAttrs.
> > > I don't think this is right at all. An argument being noundef has nothing 
> > > to do at all with memory sanitization. It affects optimization, too. 
> > > SanitizeMemoryParamRetval should only affect what MemorySanitizerPass 
> > > does with noundef attributes.
> > 
> > What is the point of running SanitizeMemoryParamRetval==1 && 
> > EnableNoundefAttrs==0?
> > Are we going to show warning or error and ask to set thee redundant 
> > options? Then why not just automate this for the user?
> > Also please note that HasStrictReturn already check SanitizerKind::Memory 
> > and adjust NoUndef attributes.
> > 
> > On the other hand, if EnableNoundefAttrs is going to be true by default in 
> > future, this conversation is not important, we can have two flags.
> > 
> > Is the problem the form of the new code?
> >   - instead of introducing a single new flag to flip 4 others, should there 
> > be 1 flag to pick up the CodeGen changes, and another for the Sanitizer?  
> > (Is 2 flags better than 4?)
> >   - another option is have the changes propagate in the other direction: 
> > use the flag (-fsanitize-memory-param-retval), to passes along '-mllvm 
> > -enable_noundef_analysis=1' to CodeGen (via SanitizerArgs.addArgs).
> > 
> > OR is there a problem forcing EnableNoundefAttrs based on an "unrelated" 
> > flag?
> >   - then leave existing code, just don't do anything fancy to change 
> > EnableNoundefAttrs.
> 
> I believe @eugenis point that user should proved both 
> -fsanitize-memory-param-retval and -enable-noundef-analysis to clang (we 
> don't need -mllvm for -enable-noundef-analysis already).
> 
> 
Sorry, I think I misunderstood the code. It makes sense for 
-fsanitize-memory-param-retval to implicitly enable noundef attrs. I agree with 
Vitaly that it is better to change the value of EnableNoundefAttrs, to avoid 
the risk of enabling only part of that feature (now or with some future change).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633

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

[PATCH] D116633: Add -fsanitize-address-param-retval to clang.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2384
+if ((CodeGenOpts.EnableNoundefAttrs ||
+ CodeGenOpts.SanitizeMemoryParamRetval) &&
+ArgNoUndef)

vitalybuka wrote:
> vitalybuka wrote:
> > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if 
> > SanitizeMemoryParamRetval is provided?
> > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if 
> > SanitizeMemoryParamRetval is provided?
> 
> To clarify, checking SanitizeMemoryParamRetval here as-is is LGTM, unless 
> @eugenis or someone else thinks EnableNoundefAttrs reset is better.
I don't think this is right at all. An argument being noundef has nothing to do 
at all with memory sanitization. It affects optimization, too. 
SanitizeMemoryParamRetval should only affect what MemorySanitizerPass does with 
noundef attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633

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

[PATCH] D116182: [ASan] Moved optimized callbacks into a separate library.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Maybe `asan_client` (like `stats_client` above), or `asan_inline`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116182

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

[PATCH] D116182: [ASan] Moved optimized callbacks into a separate library.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I don't like the name "asan_dso". DSO means "dynamic shared object", and this 
is the very opposite of that. Maybe "asan_private" or "asan_helper"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116182

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

[PATCH] D114421: [asan] Add support for disable_sanitizer_instrumentation attribute

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM




Comment at: clang/test/CodeGen/asan-globals.cpp:62
 // CHECK: ![[ATTR_GLOBAL]] = !{{{.*}}, null, null, i1 false, i1 true}
+// CHECK: ![[DISABLE_INSTR_GLOBAL]] = !{{{.*}}, null, null, i1 false, i1 true}
 // CHECK: ![[BLACKLISTED_GLOBAL]] = !{{{.*}}, null, null, i1 false, i1 true}

Does this test rely on the metadata being in the same order as the global 
declarations in the source?  Feels brittle and hard to understand - could you 
check if the regexps could be made more precise?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114421

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

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D111443

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

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/lib/Driver/ToolChain.cpp:124
+  }
+  return SanitizerArgs(*this, JobArgs, /*DiagnoseErrors=*/false);
 }

SanitizerArgs SanArgs(*this, JobArgs, !SanitizerArgsChecked);
SanitizerArgsChecked = true;
return SanArgs;




Comment at: clang/lib/Driver/ToolChains/FreeBSD.cpp:471
+bool FreeBSD::isPIEDefault(const llvm::opt::ArgList ) const {
+  return getSanitizerArgs(Args).requiresPIE();
+}

it looks like we do a lot of sanitizerargs recomputing. Is it worth it to try 
and cache it?


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

https://reviews.llvm.org/D111443

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

In D105169#3111935 , @hyeongyukim 
wrote:

>   -  long long res;
>   +  register long long res __asm__("x0");
>
> Is it okay to commit this change by myself?

Yes, go ahead!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

The approach looks reasonable to me.


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

https://reviews.llvm.org/D111443

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

You are absolutely right. X86 variant uses an "=a" constraint (rax register), 
others pin the output variable to a specific register with __asm__ declaration. 
It appears we've missed it in Aarch64.

Could you check if __asm__("x0") in the declaration of res helps?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D112098: [ASan] Added stack safety support in address sanitizer.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: llvm/include/llvm/Analysis/StackSafetyAnalysis.h:93
+  bool invalidate(Module &, const PreservedAnalyses &,
+  ModuleAnalysisManager::Invalidator &) {
+return false;

vitalybuka wrote:
> vitalybuka wrote:
> > we still can't do that, some passes can make results irrelevant
> Looks like only immutable analysis can be used through proxy, and this 
> analysis cannot be immutable.
> Maybe we have to convert Asan into ModulePass, like HWAsan.
> Any other ideas? @eugenis @pcc @morehouse 
Is the problem here that an invalidated analysis needs to be rerun, and the 
module-to-function proxy does not know how to do that?

TBH, I'm not sure how big the performance benefits of a function pass are. With 
the amount of time we've sunk in working around function pass requirements and 
I'd say just go for it.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112098

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

[PATCH] D108453: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default (2)

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D108453/new/

https://reviews.llvm.org/D108453

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

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Right, but a cache for SanitizerArgs is not enough to avoid repeated 
diagnostics, is it? Ex. if I request a non-existing sanitizer, I think I would 
get errors from host arg parsing, as well as from each of device1 and device2, 
because each device will have a unique ArgList.

Is it even possible for the driver to introduce new diagnostics in offload 
SanitizerArgs parsing? Is it possible to catch those cases ahead of time, when 
parsing SanitizerArgs for the first time, by looking at a target triple or 
something? That would be the most user friendly approach.


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

https://reviews.llvm.org/D111443

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

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Don't we want to diagnose the problems in the job's command line? What kind of 
changes can the driver do there that would affect SanitizerArgs?

I wonder if diagnostics should still be performed on the job args, but 
presented to the user differently, mainly because we can no longer refer to the 
user-provided command line?


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

https://reviews.llvm.org/D111443

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

[PATCH] D111344: [HWASan] Use tagged-globals feature on x86.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D111344/new/

https://reviews.llvm.org/D111344

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

@hyeongyukim, thank you for the summary. This looks like a great change, and a 
net positive to me. The test churn in the other patch is unfortunate, but IMHO 
unavoidable.

If no one has any further objections, 
LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

___
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.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis 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],

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.



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 custom calling convention similar to the one used by HWASan for X86.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis 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],

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.




Comment at: llvm/include/llvm/IR/Intrinsics.td:1642
+  Intrinsic<[],[llvm_ptr_ty, llvm_i8_ty, llvm_i8_ty],
+[IntrInaccessibleMemOnly, ImmArg>, 
ImmArg>]>;
+

We've just removed IntrInaccessibleMemOnly from hwasan. This needs to alias 
shadow updates.



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] D108199: [msan] Add support for disable_sanitizer_instrumentation attribute

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D108199/new/

https://reviews.llvm.org/D108199

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

[PATCH] D108029: [clang][Codegen] Introduce the no_sanitizer_instrumentation attribute

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

The new attribute should be very rare, and not something that a typical end 
user should even know about.
That's why I'd prefer if the name reflected how the attribute affects code 
generation rather than user-visible behavior: disable_sanitizer_instrumentation 
sounds perfect to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108029

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

[PATCH] D99381: [compiler-rt][hwasan] Remove __sanitizer allocation functions from hwasan interface

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

They are used here:
https://cs.android.com/android/platform/superproject/+/master:bionic/libc/bionic/malloc_common.h;l=54;drc=f3968e89cb72400951f93a2a8237ac1428d2627c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99381

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

[PATCH] D105703: [hwasan] Use stack safety analysis.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

In D105703#2887005 , @fmayer wrote:

> I removed the stack-safety-analysis-asm.c test because I don't think it 
> really adds anything and it caused problems. SGTY?

Absolutely.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703

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

[PATCH] D105703: [hwasan] Use stack safety analysis.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

In D105703#2884056 , @vitalybuka 
wrote:

> In D105703#2883666 , @eugenis wrote:
>
>> Btw Vitaly, will this work with LTO out of the box? I think we used to add 
>> pre-LTO StackSafety pass explicitly for memtag only, but it looks like that 
>> code is gone.
>
> What do you mean gone? Can you show me the patch?
>
> We probably needs something to add analysis for HWASAN as well.

https://reviews.llvm.org/D80771




Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:427
+ bool DisableOptimization) {
   assert(!CompileKernel || Recover);
+  return new HWAddressSanitizerLegacyPass(CompileKernel, Recover,

vitalybuka wrote:
> @eugenis unrelated to the patch, but why do we this args if then we 
> assert(!CompileKernel || Recover);
why not? This forbids CompileKernel && !Recover, how else would you represent 
the 3 remaining combinations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703

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

[PATCH] D105703: [hwasan] Use stack safety analysis.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Btw Vitaly, will this work with LTO out of the box? I think we used to add 
pre-LTO StackSafety pass explicitly for memtag only, but it looks like that 
code is gone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703

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

[PATCH] D105703: [hwasan] Use stack safety analysis.

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1174
+CompileKernel, Recover,
+/*IsOptNull=*/CodeGenOpts.OptimizationLevel == 0));
   }

DisableOptimization=



Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:407
   bool Recover;
+  bool DisableOptimization;
 };

fmayer wrote:
> fmayer wrote:
> > eugenis wrote:
> > > No need to pass this down, just look at OptimizeNone function attribute.
> > Interesting, the Aarch64StackTagging code does pass this down, do you know 
> > why?
> Also we really should be using this at least in the getAnalysisUsage, which 
> Vitaly's change made unconditional. Correct?
Ah right. Legacy pass needs to declare its analysis dependencies in advance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703

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

[PATCH] D105703: [hwasan] Use stack safety analysis.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:407
   bool Recover;
+  bool DisableOptimization;
 };

No need to pass this down, just look at OptimizeNone function attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703

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

[PATCH] D105703: [hwasan] Use stack safety analysis.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: llvm/test/CodeGen/hwasan-stack-safety-analysis-asm.c:1
+// RUN: %clang -fsanitize=hwaddress -target aarch64-linux-gnu -S -mllvm 
-hwasan-use-stack-safety=true -mllvm -hwasan-generate-tags-with-calls -O2 %s -o 
- | FileCheck %s --check-prefix=SAFETY
+// RUN: %clang -fsanitize=hwaddress -target aarch64-linux-gnu -S -mllvm 
-hwasan-use-stack-safety=false -mllvm -hwasan-generate-tags-with-calls -O2 %s 
-o - | FileCheck %s --check-prefix=NOSAFETY

You can't use %clang in LLVM tests. There may be no clang. Use opt to invoke 
the hwasan pass in isolation, see the tests under 
test/Instrumentation/HWAddressSanitizer/.

The reason I did not say anything about the tests was because only in clang we 
can test this thing end-to-end.
Perhaps an LLVM test for instrumentation + a clang test for the passmanager 
(use -fdebug-pass-manager and -mllvm -debug-pass=Structure for new/old).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703

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

[PATCH] D105703: [hwasan] Use stack safety analysis.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

The analysis should not run at -O0.
Otherwise, LGTM.
What kind of code size improvement do you see?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703

___
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.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

LGTM


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] D104553: [compiler-rt][hwasan] Add InitState options to thread initialization

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D104553/new/

https://reviews.llvm.org/D104553

___
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

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

In D103845#2806211 , @leonardchan 
wrote:

> 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.

This makes sense to me. This boils down to ListOfModules being unimplemented on 
Fuschia, right?


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] D103564: [NFC][compiler-rt][hwasan] Wrap vfork interceptor with HWASAN_WITH_INTERCEPTORS

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

s/vfork/fork/ in the description

I'm afraid removing the fork interceptor will break Android. This is a pretty 
uncommon condition, but still. It is interesting to note that ASan does not 
have this fork protection, see https://github.com/google/sanitizers/issues/774 
for some history.

I wonder if this could be replaced with pthread_atfork. The order of callback 
execution seems right, as long as hwasan is registered first. Or, if you want 
to make progress, add SANITIZER_ANDROID ifdef somewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103564

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

[PATCH] D102592: [sanitizer] Caught global buffer underflow for first variable

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I think it is too hit-and-miss to add even under a flag. It's just not the 
right approach - but I also don't know what the right approach would be.

Perhaps adding a small left redzone for all globals and reducing the right 
redzone to keep the total size under control? This way when most globals are 
instrumented we get approximately the same amount of redzone between any 2 of 
them, but also a little on the left of the first one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102592

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

[PATCH] D102592: [sanitizer] Caught global buffer underflow for first variable

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

You are adding a new global to every translation unit.

- Private linkage would not allow them to be merged, this can have significant 
binary size and RAM overhead.
- There is no guarantee that any of these globals will end up to the left of 
any sanitized globals. With -fdata-sections, the linker is free to reorder.
- It is also not referenced from anything, so -gc-sections is likely to kill it.

It looks like this will only work in very limited circumstances.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102592

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

[PATCH] D102469: [sanitizer] Reduce redzone size for small size global objects

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102469

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

[PATCH] D102288: [HWASan] Add -fsanitize=lam flag and enable HWASan to use it.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/include/clang/Driver/Options.td:4172
+// on x86_64 and is subject to change in the future.  For example, we may want
+// to distinguish between LAM48 and LAM57 at some point.
+def mlam : Flag<["-"], "mlam">, Group;

Hmm this might be a good argument to go with something else for now.
What would -mlam actually affect, besides hwasan? Significant bits computation?

If we see LAM as future production mode for HWASan, and aliasing - more of a 
temporary, proof-of-concept thing, then how about 
-fexperimental-sanitize-hwaddress-aliasing flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102288

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

[PATCH] D102288: [HWASan] Add -fsanitize=lam flag and enable HWASan to use it.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/include/clang/Basic/Sanitizers.def:55-59
+// Utilize Intel LAM in sanitizers.  Currently only used in combination with
+// -fsanitize=hwaddress.  This is an experimental flag which may be removed in
+// the future.
+// TODO: Use -mlam instead, if/when it is supported by clang.
+SANITIZER("lam", LAM)

vitalybuka wrote:
> morehouse wrote:
> > vitalybuka wrote:
> > > if it's experimental, why not just "-fsanitize=hwaddress -mllvm 
> > > -havasan-lam=1" ?
> > Well, `-mllvm` indicates a flag for LLVM, but we need to change the Clang 
> > behavior to link with the LAM-enabled HWASan runtime.  It seems to me that 
> > we should use a flag directed to Clang for this.
> > 
> > Maybe it's possible to parse the `-mllvm` flag before the point where we 
> > need to choose a runtime (I'm not sure), but it seems simpler to do it this 
> > way.
> I see, I expected you convince you to keep same runtime lib, but I see your 
> reasoning on the another patch.
> 
> What do you thing about still keeping it hwasan and add hwasan internal flag 
> like we added for asan
> e.g. -fsanitize-hwasan-use-lam=1. Later when -mlam is available we will drop 
> this flag.
> 
I agree, this is not a new "sanitizer", just a different kind of hwasan.

Can we simply add "-mlam" right now, with the single purpose of selecting the 
new hwasan mode? It can grow more functionality later.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102288

___
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

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Right, our commit process is a bit strange. Ideally, the change author should 
initiate commit after they have the necessary approvals, and then be 
responsible to deal with any fallout in a timely manner. As things are, the 
author does not control the timing of the commit, and the committer does not 
get the bot messages.


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] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I fixed another one in 
https://reviews.llvm.org/rG561f4b9087457e9ea3cf4aeb57dcd507e2fa6258
Please watch the bots after you land changes like this one and don't let them 
stay broken for an entire day!


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] D99368: [compiler-rt][hwasan] Add C++17 new/delete operators with alignment

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM




Comment at: compiler-rt/lib/hwasan/hwasan_new_delete.cpp:104
+INTERCEPTOR_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE void operator delete(
+void *ptr, std::align_val_t align)NOEXCEPT {
+  OPERATOR_DELETE_BODY;

missing space before NOEXCEPT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99368

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

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I'm not working on this currently, but I would appreciate and support such 
change. It's a big diff, but IMHO it is the cleanest, most maintainable 
approach.

Gui, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82317

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

[PATCH] D98892: [HWASan] Mention x86_64 aliasing mode in design doc.

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/docs/HardwareAssistedAddressSanitizerDesign.rst:279
+
+HWASAN's approach is not applicable to 32-bit architectures.
 

This statement seems a bit too strong. Maybe say something along these lines: 
none of the existing 32-bit platforms provide a practical way to put tag bits 
in the pointer, and the address space constraints make it hard to do anyway, so 
hwasan does not support 32-bit at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98892

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

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Any more comments/opinions?
Gui, would you like to push it to main if no one objects?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678

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

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D81678/new/

https://reviews.llvm.org/D81678

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

[PATCH] D96406: [Msan, NewPM] Reduce size of msan binaries

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM, please add a TODO for more passes




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1291
+  // general purpose optimization passes.
+  FPM.addPass(EarlyCSEPass());
+}

I did not know that MSan does not add the extra optimization passes in the new 
pass manager (see addGeneralOptsForMemorySanitizer). If I remember correctly, 
it used to give ~10% or more speedup on spec2006.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96406

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

[PATCH] D96328: [Clang, NewPM] Add KMSan support

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D96328/new/

https://reviews.llvm.org/D96328

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

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

In D81678#2503931 , @nikic wrote:

> As the discussion is spread out across multiple threads, do I understand 
> correctly that the current consensus is to introduce the 
> `-disable-noundef-analysis` flag, and explicitly add it to all the relevant 
> tests (rather than adding it to the substitutions)?

Yes, I think so.

> In any case, I'd recommend changing this patch to default 
> `-disable-noundef-analysis` to true (so you need to compile with 
> `-disable-noundef-analysis=0` to get undef attributes). The flag can then be 
> flipped together with the test changes. That should help get the main 
> technical change landed directly, and avoid the need of landing patches at 
> the same time.

This is a great idea! Aside from splitting the complexity of landing the large 
change, it also makes our downstream cleanup easier.

In that case we should probably give the flag a positive name: 
-enable-noundef-analysis=(0|1).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678

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

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

In D85788#2449299 , @aqjune wrote:

> In D85788#2444136 , @guiand wrote:
>
>> IMO it's better to just one-and-done programatically add `-Xclang 
>> -disable-noundef-analysis` to all the tests' RUN lines. That way there are 
>> no hidden flags.
>
> If a script for this can be written and people who are working for the 
> downstream project are happy with the script, this is the best approach IMO. 
> It is clear and explicit.

Yeah, I also think this would be the best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85788

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

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

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

I wonder if we could just disable unused argument warning for both

  -disable-noundef-analysis
  -Xclang -disable-noundef-analysis

I don't see any precedents for this kind of thing in the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85788

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

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

> simply naming it as %clang_noundef would be clearer,

Hmm, yeah, that's a little better.

> This new substitution is going to be used for the noundef checks in D81678 
>  only anyway.

I'm not sure what you mean by this. The substitution is used ~380 times in this 
patch alone.
In the future, any attempt to use %clang -### or -%clang -cc1as will produce 
the same unused argument warning, and it is very unclear that is can be fixed 
with %clang_bin. Note that code search for "-disable-noundef-analysis" does NOT 
lead you to the definition of %clang_bin!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85788

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

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

This change looks fine to me, but I'm slightly concerned about 
https://reviews.llvm.org/D85788 - see my last comment on that revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678

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

[PATCH] D92728: [NFC][MSan] Round up OffsetPtr in PoisonMembers

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/CodeGen/CGClass.cpp:1742
+  Context.toCharUnitsFromBits(Layout.getFieldOffset(layoutStartOffset) 
+
+  Context.getCharWidth() - 1);
+  llvm::ConstantInt *OffsetSizePtr =

vitalybuka wrote:
> vitalybuka wrote:
> > morehouse wrote:
> > > How does this interact with bit fields?
> > I guess existing code work because is bit fields are trivial and the one 
> > aligned to the char is going to be layoutStartOffset
> > I assume if we call this function for layoutStartOffset pointing to the 
> > bitfield in the middle of char it will poison entire byte which is 
> > incorrect.
> > With the patch it will not poison such chars: now it will be either all 
> > bits or nothing which I believe better.
> toCharUnitsFromBits already down aligns by definition: it's just a 
> bitsoffset/charwidth
> 
> regarding assert: I am not 100% that this is not possible, maybe some 
> corner-case in language allows that.
> if so, it's better not to poison partial byte at all than poison it 
> completely.
> 
> I assume if we call this function for layoutStartOffset pointing to the 
> bitfield in the middle of char it will poison entire byte which is incorrect.
Right. We never do this, of course, because the previous non-trivial field can 
not possibly end in the middle of a char.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92728

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

[PATCH] D92728: [NFC][MSan] Round up OffsetPtr in PoisonMembersgetFieldOffset(layoutStartOffset) for current calleds is expected topoint to the first trivial field or the one which follows non-trivial

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Don't you want to similarly align down PoisonEnd?

But if this is something that should never happen, as your comment rightly 
suggests, wouldn't it be better to add an assert()?
The same in the case when PoisonSize < 0 - it should never happen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92728

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

[PATCH] D92727: [CodeGen][MSan] Don't use offsets of zero-sized fields

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92727

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

[PATCH] D91258: [clangd] Sanity-check array sizes read from disk before allocating them.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Hi Sam,

this patch is failing on the ubsan bot with:

[ RUN  ] SerializationTest.NoCrashOnBadArraySize
/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/index/Serialization.cpp:90:26:
 runtime error: left shift of 127 by 28 places cannot be represented in type 
'int'

  #0 0x392dd57 in clang::clangd::(anonymous namespace)::Reader::consumeVar() 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/index/Serialization.cpp:90:26
  #1 0x392dc68 in bool clang::clangd::(anonymous 
namespace)::Reader::consumeSize > >(std::__1::vector >&) 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/index/Serialization.cpp:113:17
  #2 0x3926d6a in readIncludeGraphNode 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/index/Serialization.cpp:275:13
  #3 0x3926d6a in clang::clangd::(anonymous 
namespace)::readRIFF(llvm::StringRef) 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/index/Serialization.cpp:474:18
  #4 0x3926111 in clang::clangd::readIndexFile(llvm::StringRef) 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/index/Serialization.cpp:676:12
  #5 0x1e5b77c in clang::clangd::(anonymous 
namespace)::SerializationTest_NoCrashOnBadArraySize_Test::TestBody() 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/unittests/SerializationTests.cpp:380:24
  #6 0x20589f9 in testing::Test::Run() 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2474:5
  #7 0x205993b in testing::TestInfo::Run() 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2656:11
  #8 0x205a4e2 in testing::TestCase::Run() 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2774:28
  #9 0x20619b2 in testing::internal::UnitTestImpl::RunAllTests() 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4649:43
  #10 0x20613c9 in testing::UnitTest::Run() 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4257:10
  #11 0x2051273 in RUN_ALL_TESTS 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2233:46
  #12 0x2051273 in main 
/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
  #13 0x7fd11479d09a in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
  #14 0x1a330d9 in _start 
(/b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/tools/clang/tools/extra/clangd/unittests/ClangdTests+0x1a330d9)

It looks like the corrupt input in your test case is triggering a preexisting 
bug in clangd.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91258

___
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.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM
I was confused about the ABI statement in the description at the first glance - 
could you reword it to make it clear that HWASan ABI is not affected?


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.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90424

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

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

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D89766/new/

https://reviews.llvm.org/D89766

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

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis updated this revision to Diff 298692.
eugenis added a comment.

fix type size check for vscale types


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/attr-noundef.cpp
  clang/test/CodeGen/indirect-noundef.cpp
  clang/test/lit.cfg.py
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -28,9 +28,9 @@
 from UpdateTestChecks import common
 
 SUBST = {
-'%clang': [],
-'%clang_cc1': ['-cc1'],
-'%clangxx': ['--driver-mode=g++'],
+'%clang': ['-Xclang -disable-noundef-analysis'],
+'%clang_cc1': ['-cc1', '-disable-noundef-analysis'],
+'%clangxx': ['--driver-mode=g++', '-disable-noundef-analysis'],
 }
 
 def get_line2spell_and_mangled(args, clang_args):
@@ -161,8 +161,8 @@
   try:
 builtin_include_dir = subprocess.check_output(
   [args.clang, '-print-file-name=include']).decode().strip()
-SUBST['%clang_cc1'] = ['-cc1', '-internal-isystem', builtin_include_dir,
-   '-nostdsysteminc']
+SUBST['%clang_cc1'] += ['-internal-isystem', builtin_include_dir,
+'-nostdsysteminc']
   except subprocess.CalledProcessError:
 common.warn('Could not determine clang builtins directory, some tests '
 'might not update correctly.')
Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -41,7 +41,8 @@
 
 llvm_config.use_default_substitutions()
 
-llvm_config.use_clang()
+llvm_config.use_clang(cc_additional_flags=['-Xclang -disable-noundef-analysis'],
+  cc1_additional_flags=['-disable-noundef-analysis'])
 
 config.substitutions.append(
 ('%src_include_dir', config.clang_src_dir + '/include'))
Index: clang/test/CodeGen/indirect-noundef.cpp
===
--- /dev/null
+++ clang/test/CodeGen/indirect-noundef.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_bin -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm -o - %s | FileCheck %s
+
+union u1 {
+  int val;
+};
+
+// CHECK: @indirect_callee_int_ptr = global i32 (i32)*
+int (*indirect_callee_int_ptr)(int);
+// CHECK: @indirect_callee_union_ptr = global i32 (i32)*
+union u1 (*indirect_callee_union_ptr)(union u1);
+
+// CHECK-LABEL: define noundef i32 @{{.*}}indirect_callee_int{{.*}}(i32 noundef %
+int indirect_callee_int(int a) { return a; }
+// CHECK-LABEL: define i32 @{{.*}}indirect_callee_union{{.*}}(i32 %
+union u1 indirect_callee_union(union u1 a) {
+  return a;
+}
+
+int main() {
+  // CHECK: call noundef i32 @{{.*}}indirect_callee_int{{.*}}(i32 noundef 0)
+  indirect_callee_int(0);
+  // CHECK: call i32 @{{.*}}indirect_callee_union{{.*}}(i32 %
+  indirect_callee_union((union u1){0});
+
+  indirect_callee_int_ptr = indirect_callee_int;
+  indirect_callee_union_ptr = indirect_callee_union;
+
+  // CHECK: call noundef i32 %{{.*}}(i32 noundef 0)
+  indirect_callee_int_ptr(0);
+  // CHECK: call i32 %{{.*}}(i32 %
+  indirect_callee_union_ptr((union u1){});
+
+  return 0;
+}
Index: clang/test/CodeGen/attr-noundef.cpp
===
--- /dev/null
+++ clang/test/CodeGen/attr-noundef.cpp
@@ -0,0 +1,163 @@
+// RUN: %clang_bin -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-INTEL
+// RUN: %clang_bin -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-INTEL
+// RUN: %clang_bin -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-AARCH
+
+// Passing structs by value
+// TODO: No structs may currently be marked noundef
+
+namespace check_structs {
+struct Trivial {
+  int a;
+};
+Trivial ret_trivial() { return {}; }
+void pass_trivial(Trivial e) {}
+// CHECK-INTEL: define i32 @{{.*}}ret_trivial
+// CHECK-AARCH: define i64 @{{.*}}ret_trivial
+// CHECK-INTEL: define void @{{.*}}pass_trivial{{.*}}(i32 %
+// CHECK-AARCH: define void @{{.*}}pass_trivial{{.*}}(i64 %
+
+struct NoCopy {
+  int a;
+  NoCopy(NoCopy &) = delete;
+};
+NoCopy ret_nocopy() { return {}; }
+void pass_nocopy(NoCopy e) {}
+// CHECK: define {{(dso_local)?}}void @{{.*}}ret_nocopy{{.*}}(%"struct.check_structs::NoCopy"* noalias sret align 4 %
+// CHECK: define void @{{.*}}pass_nocopy{{.*}}(%"struct.check_structs::NoCopy"* noundef %
+
+struct Huge {
+  int a[1024];
+};

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I don't like the %clang_bin substitution - imho it's unclear for the test 
authors when to use it instead of %clang, but I can't come up with a better 
idea.

Is it true that %clang_bin is only necessary when the automatically added 
-disable-noundef-analysis flag triggers an unused argument warning, or are 
there other reasons?

I wonder it can be avoided by

- disable noundef analysis by default in cc1
- always add -enable-noundef-analysis in the driver when invoking cc1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85788

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

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis 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/D85788/new/

https://reviews.llvm.org/D85788

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

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

[Evgenii Stepanov 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 rG66cf68ed4678: [docs] Update ControlFlowIntegrity.rst. 
(authored by eugenis).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87717

Files:
  clang/docs/ControlFlowIntegrity.rst


Index: clang/docs/ControlFlowIntegrity.rst
===
--- clang/docs/ControlFlowIntegrity.rst
+++ clang/docs/ControlFlowIntegrity.rst
@@ -76,8 +76,8 @@
 to use all schemes except for non-virtual member function call and indirect 
call
 checking.
 
-Remember that you have to provide ``-flto`` if at least one CFI scheme is
-enabled.
+Remember that you have to provide ``-flto`` or ``-flto=thin`` if at
+least one CFI scheme is enabled.
 
 Trapping and Diagnostics
 
@@ -217,7 +217,8 @@
 shared library boundaries are handled as if the callee was not compiled with
 ``-fsanitize=cfi-icall``.
 
-This scheme is currently only supported on the x86 and x86_64 architectures.
+This scheme is currently supported on a limited set of targets: x86,
+x86_64, arm, arch64 and wasm.
 
 ``-fsanitize-cfi-icall-generalize-pointers``
 
@@ -368,7 +369,7 @@
 Use **-f[no-]sanitize-cfi-cross-dso** to enable the cross-DSO control
 flow integrity mode, which allows all CFI schemes listed above to
 apply across DSO boundaries. As in the regular CFI, each DSO must be
-built with ``-flto``.
+built with ``-flto`` or ``-flto=thin``.
 
 Normally, CFI checks will only be performed for classes that have hidden LTO
 visibility. With this flag enabled, the compiler will emit cross-DSO CFI


Index: clang/docs/ControlFlowIntegrity.rst
===
--- clang/docs/ControlFlowIntegrity.rst
+++ clang/docs/ControlFlowIntegrity.rst
@@ -76,8 +76,8 @@
 to use all schemes except for non-virtual member function call and indirect call
 checking.
 
-Remember that you have to provide ``-flto`` if at least one CFI scheme is
-enabled.
+Remember that you have to provide ``-flto`` or ``-flto=thin`` if at
+least one CFI scheme is enabled.
 
 Trapping and Diagnostics
 
@@ -217,7 +217,8 @@
 shared library boundaries are handled as if the callee was not compiled with
 ``-fsanitize=cfi-icall``.
 
-This scheme is currently only supported on the x86 and x86_64 architectures.
+This scheme is currently supported on a limited set of targets: x86,
+x86_64, arm, arch64 and wasm.
 
 ``-fsanitize-cfi-icall-generalize-pointers``
 
@@ -368,7 +369,7 @@
 Use **-f[no-]sanitize-cfi-cross-dso** to enable the cross-DSO control
 flow integrity mode, which allows all CFI schemes listed above to
 apply across DSO boundaries. As in the regular CFI, each DSO must be
-built with ``-flto``.
+built with ``-flto`` or ``-flto=thin``.
 
 Normally, CFI checks will only be performed for classes that have hidden LTO
 visibility. With this flag enabled, the compiler will emit cross-DSO CFI
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis updated this revision to Diff 295888.
eugenis added a comment.

fix a typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87717

Files:
  clang/docs/ControlFlowIntegrity.rst


Index: clang/docs/ControlFlowIntegrity.rst
===
--- clang/docs/ControlFlowIntegrity.rst
+++ clang/docs/ControlFlowIntegrity.rst
@@ -76,8 +76,8 @@
 to use all schemes except for non-virtual member function call and indirect 
call
 checking.
 
-Remember that you have to provide ``-flto`` if at least one CFI scheme is
-enabled.
+Remember that you have to provide ``-flto`` or ``-flto=thin`` if at
+least one CFI scheme is enabled.
 
 Trapping and Diagnostics
 
@@ -217,7 +217,8 @@
 shared library boundaries are handled as if the callee was not compiled with
 ``-fsanitize=cfi-icall``.
 
-This scheme is currently only supported on the x86 and x86_64 architectures.
+This scheme is currently supported on a limited set of targets: x86,
+x86_64, arm, arch64 and wasm.
 
 ``-fsanitize-cfi-icall-generalize-pointers``
 
@@ -368,7 +369,7 @@
 Use **-f[no-]sanitize-cfi-cross-dso** to enable the cross-DSO control
 flow integrity mode, which allows all CFI schemes listed above to
 apply across DSO boundaries. As in the regular CFI, each DSO must be
-built with ``-flto``.
+built with ``-flto`` or ``-flto=thin``.
 
 Normally, CFI checks will only be performed for classes that have hidden LTO
 visibility. With this flag enabled, the compiler will emit cross-DSO CFI


Index: clang/docs/ControlFlowIntegrity.rst
===
--- clang/docs/ControlFlowIntegrity.rst
+++ clang/docs/ControlFlowIntegrity.rst
@@ -76,8 +76,8 @@
 to use all schemes except for non-virtual member function call and indirect call
 checking.
 
-Remember that you have to provide ``-flto`` if at least one CFI scheme is
-enabled.
+Remember that you have to provide ``-flto`` or ``-flto=thin`` if at
+least one CFI scheme is enabled.
 
 Trapping and Diagnostics
 
@@ -217,7 +217,8 @@
 shared library boundaries are handled as if the callee was not compiled with
 ``-fsanitize=cfi-icall``.
 
-This scheme is currently only supported on the x86 and x86_64 architectures.
+This scheme is currently supported on a limited set of targets: x86,
+x86_64, arm, arch64 and wasm.
 
 ``-fsanitize-cfi-icall-generalize-pointers``
 
@@ -368,7 +369,7 @@
 Use **-f[no-]sanitize-cfi-cross-dso** to enable the cross-DSO control
 flow integrity mode, which allows all CFI schemes listed above to
 apply across DSO boundaries. As in the regular CFI, each DSO must be
-built with ``-flto``.
+built with ``-flto`` or ``-flto=thin``.
 
 Normally, CFI checks will only be performed for classes that have hidden LTO
 visibility. With this flag enabled, the compiler will emit cross-DSO CFI
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

We really need to do something about this.
How about a change that adds -fdisable-noundef-analysis to every RUN line with 
%clang?
(-) We are not testing exactly the same mode that is used by the users - but 
that's already true for many other flags that clang driver passes to -cc1!
(+) Easy to automate, an update script can be provided to downstream users.
(+) Less "magic" than the llvm-lit idea (4 comments above)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82317

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

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

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis created this revision.
eugenis added a reviewer: pcc.
Herald added a subscriber: dexonsmith.
Herald added a project: clang.
eugenis requested review of this revision.
Herald added a subscriber: aheejin.

Expand the list of targets that support cfi-icall.
Add ThinLTO everywhere LTO is mentioned. AFAIK all CFI features are
supported with ThinLTO.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87717

Files:
  clang/docs/ControlFlowIntegrity.rst


Index: clang/docs/ControlFlowIntegrity.rst
===
--- clang/docs/ControlFlowIntegrity.rst
+++ clang/docs/ControlFlowIntegrity.rst
@@ -76,8 +76,8 @@
 to use all schemes except for non-virtual member function call and indirect 
call
 checking.
 
-Remember that you have to provide ``-flto`` if at least one CFI scheme is
-enabled.
+Remember that you have to provide ``-flto`` or ``-flto=thin`` if at
+least one CFI scheme is enabled.
 
 Trapping and Diagnostics
 
@@ -217,7 +217,8 @@
 shared library boundaries are handled as if the callee was not compiled with
 ``-fsanitize=cfi-icall``.
 
-This scheme is currently only supported on the x86 and x86_64 architectures.
+This scheme is currently supported on a limited set of targets: x86,
+x86_64, arm, arch64 and wasm.
 
 ``-fsanitize-cfi-icall-generalize-pointers``
 
@@ -368,7 +369,7 @@
 Use **-f[no-]sanitize-cfi-cross-dso** to enable the cross-DSO control
 flow integrity mode, which allows all CFI schemes listed above to
 apply across DSO boundaries. As in the regular CFI, each DSO must be
-built with ``-flto``.
+built with ``-flto`` or ``flto=thin``.
 
 Normally, CFI checks will only be performed for classes that have hidden LTO
 visibility. With this flag enabled, the compiler will emit cross-DSO CFI


Index: clang/docs/ControlFlowIntegrity.rst
===
--- clang/docs/ControlFlowIntegrity.rst
+++ clang/docs/ControlFlowIntegrity.rst
@@ -76,8 +76,8 @@
 to use all schemes except for non-virtual member function call and indirect call
 checking.
 
-Remember that you have to provide ``-flto`` if at least one CFI scheme is
-enabled.
+Remember that you have to provide ``-flto`` or ``-flto=thin`` if at
+least one CFI scheme is enabled.
 
 Trapping and Diagnostics
 
@@ -217,7 +217,8 @@
 shared library boundaries are handled as if the callee was not compiled with
 ``-fsanitize=cfi-icall``.
 
-This scheme is currently only supported on the x86 and x86_64 architectures.
+This scheme is currently supported on a limited set of targets: x86,
+x86_64, arm, arch64 and wasm.
 
 ``-fsanitize-cfi-icall-generalize-pointers``
 
@@ -368,7 +369,7 @@
 Use **-f[no-]sanitize-cfi-cross-dso** to enable the cross-DSO control
 flow integrity mode, which allows all CFI schemes listed above to
 apply across DSO boundaries. As in the regular CFI, each DSO must be
-built with ``-flto``.
+built with ``-flto`` or ``flto=thin``.
 
 Normally, CFI checks will only be performed for classes that have hidden LTO
 visibility. With this flag enabled, the compiler will emit cross-DSO CFI
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

In D86000#2219322 , @jfb wrote:

> In D86000#2219288 , @vsk wrote:
>
>> It'd be nice to fold the new check into an existing sanitizer group to bring 
>> this to a wider audience. Do you foresee adoption issues for existing 
>> -fsanitize=integer adopters? Fwiw some recently-added implicit conversion 
>> checks were folded in without much/any pushback.
>
> `integer` does "not actually UB checks", right? I can certainly put it in 
> there if you think I won't get yelled at 

I'd support this.
"integer" includes unsigned-integer-overflow, it's only recommended to the 
truly fearless developers :)




Comment at: clang/test/Driver/fsanitize.c:911
+// CHECK-unsigned-shift-base-RECOVER-NOT: "-fsanitize-trap=unsigned-shift-base"
+// CHECK-unsigned-shift-base-NORECOVER-NOT: 
"-fno-sanitize-recover=unsigned-shift-base"
+// CHECK-unsigned-shift-base-NORECOVER-NOT: 
"-fsanitize-recover=unsigned-shift-base"

vsk wrote:
> jfb wrote:
> > vsk wrote:
> > > Not sure I follow this one. Why is 'NORECOVER' not expecting to see 
> > > "-fno-sanitize-recover=unsigned-shift-base"?
> > I have no idea! Other parts of this file do this:
> > ```
> > // CHECK-implicit-conversion-NORECOVER-NOT: 
> > "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
> >  // ???
> > // CHECK-implicit-integer-arithmetic-value-change-NORECOVER-NOT: 
> > "-fno-sanitize-recover={{((implicit-signed-integer-truncation|implicit-integer-sign-change),?){2}"}}
> >  // ???
> > // CHECK-implicit-integer-truncation-NORECOVER-NOT: 
> > "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
> >  // ???
> > ```
> > I was hoping someone who's touched this before would know.
> The CHECK-implicit-conversion-NORECOVER-NOT regex looks fishy. There's more 
> parens in there than I'd expect: perhaps that explains why the test passes?
> 
> Just above your change, I see something that's closer to what I expect:
> 
> ```
> // RUN: %clang -fsanitize=float-divide-by-zero %s 
> -fno-sanitize-recover=float-divide-by-zero -### 2>&1 | FileCheck %s 
> --check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-NORECOVER
> ...
> // CHECK-DIVBYZERO-NORECOVER-NOT: "-fsanitize-recover=float-divide-by-zero"
> ```
I think that + the following line mean that the frontend depends on the default 
behavior (neither recover nor no-recover).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000

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

[PATCH] D85559: [MSAN] Reintroduce libatomic load/store instrumentation

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3573
+  case LibFunc_atomic_load:
+if (!isa(CB)) {
+  llvm::errs() << "MSAN -- cannot instrument invoke of libatomic load."

guiand wrote:
> eugenis wrote:
> > Probably need the same check for atomic_store.
> Although it would be good for consistency I don't think it's strictly needed 
> since with `atomic_store` we don't need to use `getNextNode()`
ah, good point. LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85559

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