[PATCH] D157445: [CodeGen][UBSan] Add support for handling attributed functions in getUBSanFunctionTypeHash.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: clang/test/CodeGen/ubsan-function-attributed.c:4
+//Make sure compiler does not crash inside getUBSanFunctionTypeHash while 
compiling this
+long __attribute__((ms_abi)) f() {}

MaskRay wrote:
> https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-too-little
> 
> "A regression test which simply runs the compiler and expects it not to crash 
> has low utilization. It is often better to additionally test some properties 
> of the produced object file."
> 
> I think it's better to define another function without 
> `__attribute__((ms_abi))`, and check that the two functions have the same 
> type hash.
I agree with this and particularly this sounds like a good approach:
> I think it's better to define another function without 
> __attribute__((ms_abi)), and check that the two functions have the same type 
> hash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157445

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

[PATCH] D148967: Disable atexit()-based lowering when LTO'ing kernel/kext code

[Julian Lettner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc3f0153ec27a: [MachO] Disable atexit()-based lowering when 
LTOing kernel/kext code (authored by yln).

Changed prior to commit:
  https://reviews.llvm.org/D148967?vs=515929=516862#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148967

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-ld-lto.c
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll


Index: llvm/test/CodeGen/ARM/ctors_dtors.ll
===
--- llvm/test/CodeGen/ARM/ctors_dtors.ll
+++ llvm/test/CodeGen/ARM/ctors_dtors.ll
@@ -1,4 +1,5 @@
 ; RUN: llc < %s -mtriple=arm-apple-darwin  | FileCheck %s -check-prefix=DARWIN
+; RUN: llc < %s -mtriple=arm-apple-darwin 
-disable-atexit-based-global-dtor-lowering  | FileCheck %s 
-check-prefix=DARWIN-LEGACY
 ; RUN: llc < %s -mtriple=arm-linux-gnu -target-abi=apcs  | FileCheck %s 
-check-prefix=ELF
 ; RUN: llc < %s -mtriple=arm-linux-gnueabi | FileCheck %s -check-prefix=GNUEABI
 
@@ -7,6 +8,10 @@
 ; DARWIN: .section __DATA,__mod_init_func,mod_init_funcs
 ; DARWIN-NOT: __mod_term_func
 
+; DARWIN-LEGACY-NOT: atexit
+; DARWIN-LEGACY: .section  __DATA,__mod_init_func,mod_init_funcs
+; DARWIN-LEGACY: .section  __DATA,__mod_term_func,mod_term_funcs
+
 ; ELF: .section .ctors,"aw",%progbits
 ; ELF: .section .dtors,"aw",%progbits
 
Index: llvm/lib/CodeGen/TargetPassConfig.cpp
===
--- llvm/lib/CodeGen/TargetPassConfig.cpp
+++ llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -100,6 +100,9 @@
 cl::desc("Disable Copy Propagation pass"));
 static cl::opt 
DisablePartialLibcallInlining("disable-partial-libcall-inlining",
 cl::Hidden, cl::desc("Disable Partial Libcall Inlining"));
+static cl::opt DisableAtExitBasedGlobalDtorLowering(
+"disable-atexit-based-global-dtor-lowering", cl::Hidden,
+cl::desc("For MachO, disable atexit()-based global destructor lowering"));
 static cl::opt EnableImplicitNullChecks(
 "enable-implicit-null-checks",
 cl::desc("Fold null checks into faulting memory operations"),
@@ -878,7 +881,8 @@
 
   // For MachO, lower @llvm.global_dtors into @llvm.global_ctors with
   // __cxa_atexit() calls to avoid emitting the deprecated __mod_term_func.
-  if (TM->getTargetTriple().isOSBinFormatMachO())
+  if (TM->getTargetTriple().isOSBinFormatMachO() &&
+  !DisableAtExitBasedGlobalDtorLowering)
 addPass(createLowerGlobalDtorsLegacyPass());
 
   // Make sure that no unreachable blocks are instruction selected.
Index: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
===
--- llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -1202,7 +1202,12 @@
 
 MCSection *TargetLoweringObjectFileMachO::getStaticDtorSection(
 unsigned Priority, const MCSymbol *KeySym) const {
-  report_fatal_error("@llvm.global_dtors should have been lowered already");
+  return StaticDtorSection;
+  // In userspace, we lower global destructors via atexit(), but kernel/kext
+  // environments do not provide this function so we still need to support the
+  // legacy way here.
+  // See the -disable-atexit-based-global-dtor-lowering CodeGen flag for more
+  // context.
 }
 
 void TargetLoweringObjectFileMachO::emitModuleMetadata(MCStreamer ,
Index: clang/test/Driver/darwin-ld-lto.c
===
--- clang/test/Driver/darwin-ld-lto.c
+++ clang/test/Driver/darwin-ld-lto.c
@@ -40,3 +40,11 @@
 // GISEL: {{ld(.exe)?"}}
 // GISEL: "-mllvm" "-global-isel"
 // GISEL: "-mllvm" "-global-isel-abort=0"
+
+
+// Check that we disable atexit()-based global destructor lowering when
+// compiling/linking for kernel/kext/freestanding.
+// RUN: %clang -target arm64-apple-darwin %s -flto -fapple-kext -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEXT %s
+// KEXT: {{ld(.exe)?"}}
+// KEXT: "-mllvm" "-disable-atexit-based-global-dtor-lowering"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -393,6 +393,13 @@
 }
   }
 
+  if (Args.hasArg(options::OPT_mkernel) ||
+  Args.hasArg(options::OPT_fapple_kext) ||
+  Args.hasArg(options::OPT_ffreestanding)) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-disable-atexit-based-global-dtor-lowering");
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_prebind);
   Args.AddLastArg(CmdArgs, options::OPT_noprebind);
   Args.AddLastArg(CmdArgs, options::OPT_nofixprebinding);


Index: llvm/test/CodeGen/ARM/ctors_dtors.ll

[PATCH] D148967: WIP: Disable atexit()-based lowering when LTO'ing kernel/kext code

[Julian Lettner via Phabricator via cfe-commits]

yln created this revision.
Herald added subscribers: llvm-commits, cfe-commits, ormris, steven_wu, 
hiraditya, inglorion.
Herald added projects: clang, LLVM, All.
yln requested review of this revision.
Herald added a subscriber: MaskRay.

rdar://93536111


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148967

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-ld-lto.c
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll


Index: llvm/test/CodeGen/ARM/ctors_dtors.ll
===
--- llvm/test/CodeGen/ARM/ctors_dtors.ll
+++ llvm/test/CodeGen/ARM/ctors_dtors.ll
@@ -1,4 +1,5 @@
 ; RUN: llc < %s -mtriple=arm-apple-darwin  | FileCheck %s -check-prefix=DARWIN
+; RUN: llc < %s -mtriple=arm-apple-darwin 
-disable-atexit-based-global-dtor-lowering  | FileCheck %s 
-check-prefix=DARWIN-LEGACY
 ; RUN: llc < %s -mtriple=arm-linux-gnu -target-abi=apcs  | FileCheck %s 
-check-prefix=ELF
 ; RUN: llc < %s -mtriple=arm-linux-gnueabi | FileCheck %s -check-prefix=GNUEABI
 
@@ -7,6 +8,10 @@
 ; DARWIN: .section __DATA,__mod_init_func,mod_init_funcs
 ; DARWIN-NOT: __mod_term_func
 
+; DARWIN-LEGACY-NOT: atexit
+; DARWIN-LEGACY: .section  __DATA,__mod_init_func,mod_init_funcs
+; DARWIN-LEGACY: .section  __DATA,__mod_term_func,mod_term_funcs
+
 ; ELF: .section .ctors,"aw",%progbits
 ; ELF: .section .dtors,"aw",%progbits
 
Index: llvm/lib/CodeGen/TargetPassConfig.cpp
===
--- llvm/lib/CodeGen/TargetPassConfig.cpp
+++ llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -100,6 +100,9 @@
 cl::desc("Disable Copy Propagation pass"));
 static cl::opt 
DisablePartialLibcallInlining("disable-partial-libcall-inlining",
 cl::Hidden, cl::desc("Disable Partial Libcall Inlining"));
+static cl::opt DisableAtExitBasedGlobalDtorLowering(
+"disable-atexit-based-global-dtor-lowering", cl::Hidden,
+cl::desc("For MachO, disable atexit()-based global destructor lowering"));
 static cl::opt EnableImplicitNullChecks(
 "enable-implicit-null-checks",
 cl::desc("Fold null checks into faulting memory operations"),
@@ -878,7 +881,8 @@
 
   // For MachO, lower @llvm.global_dtors into @llvm.global_ctors with
   // __cxa_atexit() calls to avoid emitting the deprecated __mod_term_func.
-  if (TM->getTargetTriple().isOSBinFormatMachO())
+  if (TM->getTargetTriple().isOSBinFormatMachO() &&
+  !DisableAtExitBasedGlobalDtorLowering)
 addPass(createLowerGlobalDtorsLegacyPass());
 
   // Make sure that no unreachable blocks are instruction selected.
Index: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
===
--- llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -1202,7 +1202,10 @@
 
 MCSection *TargetLoweringObjectFileMachO::getStaticDtorSection(
 unsigned Priority, const MCSymbol *KeySym) const {
-  report_fatal_error("@llvm.global_dtors should have been lowered already");
+  return StaticDtorSection;
+  // kernel/kext do not provide atexit() for lowering.  See the
+  // -disable-atexit-based-global-dtor-lowering CodeGen flag.
+  // report_fatal_error("@llvm.global_dtors should have been lowered already");
 }
 
 void TargetLoweringObjectFileMachO::emitModuleMetadata(MCStreamer ,
Index: clang/test/Driver/darwin-ld-lto.c
===
--- clang/test/Driver/darwin-ld-lto.c
+++ clang/test/Driver/darwin-ld-lto.c
@@ -40,3 +40,11 @@
 // GISEL: {{ld(.exe)?"}}
 // GISEL: "-mllvm" "-global-isel"
 // GISEL: "-mllvm" "-global-isel-abort=0"
+
+
+// Check that we disable atexit()-based global destructor lowering when
+// compiling/linking for kernel/kext/freestanding.
+// RUN: %clang -target arm64-apple-darwin %s -flto -fapple-kext -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEXT %s
+// KEXT: {{ld(.exe)?"}}
+// KEXT: "-mllvm" "-disable-atexit-based-global-dtor-lowering"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -393,6 +393,13 @@
 }
   }
 
+  if (Args.hasArg(options::OPT_mkernel) ||
+  Args.hasArg(options::OPT_fapple_kext) ||
+  Args.hasArg(options::OPT_ffreestanding)) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-disable-atexit-based-global-dtor-lowering");
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_prebind);
   Args.AddLastArg(CmdArgs, options::OPT_noprebind);
   Args.AddLastArg(CmdArgs, options::OPT_nofixprebinding);


Index: llvm/test/CodeGen/ARM/ctors_dtors.ll
===
--- llvm/test/CodeGen/ARM/ctors_dtors.ll
+++ llvm/test/CodeGen/ARM/ctors_dtors.ll
@@ -1,4 +1,5 @@
 ; RUN: llc < 

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

[Julian Lettner 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 rGe6a789ef9bb2: Remove -lower-global-dtors-via-cxa-atexit flag 
(authored by yln).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145715

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll

Index: llvm/test/CodeGen/ARM/ctors_dtors.ll
===
--- llvm/test/CodeGen/ARM/ctors_dtors.ll
+++ llvm/test/CodeGen/ARM/ctors_dtors.ll
@@ -1,5 +1,4 @@
 ; RUN: llc < %s -mtriple=arm-apple-darwin  | FileCheck %s -check-prefix=DARWIN
-; RUN: llc < %s -mtriple=arm-apple-darwin -lower-global-dtors-via-cxa-atexit=false  | FileCheck %s -check-prefix=DARWIN-OLD
 ; RUN: llc < %s -mtriple=arm-linux-gnu -target-abi=apcs  | FileCheck %s -check-prefix=ELF
 ; RUN: llc < %s -mtriple=arm-linux-gnueabi | FileCheck %s -check-prefix=GNUEABI
 
@@ -8,9 +7,6 @@
 ; DARWIN: .section	__DATA,__mod_init_func,mod_init_funcs
 ; DARWIN-NOT: __mod_term_func
 
-; DARWIN-OLD: .section	__DATA,__mod_init_func,mod_init_funcs
-; DARWIN-OLD: .section	__DATA,__mod_term_func,mod_term_funcs
-
 ; ELF: .section .ctors,"aw",%progbits
 ; ELF: .section .dtors,"aw",%progbits
 
Index: llvm/lib/CodeGen/TargetPassConfig.cpp
===
--- llvm/lib/CodeGen/TargetPassConfig.cpp
+++ llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -878,8 +878,7 @@
 
   // For MachO, lower @llvm.global_dtors into @llvm.global_ctors with
   // __cxa_atexit() calls to avoid emitting the deprecated __mod_term_func.
-  if (TM->getTargetTriple().isOSBinFormatMachO() &&
-  TM->Options.LowerGlobalDtorsViaCxaAtExit)
+  if (TM->getTargetTriple().isOSBinFormatMachO())
 addPass(createLowerGlobalDtorsLegacyPass());
 
   // Make sure that no unreachable blocks are instruction selected.
Index: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
===
--- llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -1202,11 +1202,7 @@
 
 MCSection *TargetLoweringObjectFileMachO::getStaticDtorSection(
 unsigned Priority, const MCSymbol *KeySym) const {
-  // TODO(yln): Remove -lower-global-dtors-via-cxa-atexit fallback flag
-  // (LowerGlobalDtorsViaCxaAtExit) and always issue a fatal error here.
-  if (TM->Options.LowerGlobalDtorsViaCxaAtExit)
-report_fatal_error("@llvm.global_dtors should have been lowered already");
-  return StaticDtorSection;
+  report_fatal_error("@llvm.global_dtors should have been lowered already");
 }
 
 void TargetLoweringObjectFileMachO::emitModuleMetadata(MCStreamer ,
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,7 +81,6 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
-CGOPT(bool, LowerGlobalDtorsViaCxaAtExit)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -349,12 +348,6 @@
 cl::init(false));
   CGBINDOPT(UseCtors);
 
-  static cl::opt LowerGlobalDtorsViaCxaAtExit(
-  "lower-global-dtors-via-cxa-atexit",
-  cl::desc("Lower llvm.global_dtors (global destructors) via __cxa_atexit"),
-  cl::init(true));
-  CGBINDOPT(LowerGlobalDtorsViaCxaAtExit);
-
   static cl::opt RelaxELFRelocations(
   "relax-elf-relocations",
   cl::desc(
@@ -538,7 +531,6 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
-  Options.LowerGlobalDtorsViaCxaAtExit = getLowerGlobalDtorsViaCxaAtExit();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -130,7 +130,7 @@
   HonorSignDependentRoundingFPMathOption(false), NoZerosInBSS(false),
   GuaranteedTailCallOpt(false), StackSymbolOrdering(true),
   EnableFastISel(false), EnableGlobalISel(false), UseInitArray(false),
-  LowerGlobalDtorsViaCxaAtExit(false), DisableIntegratedAS(false),
+  DisableIntegratedAS(false),
   RelaxELFRelocations(true), FunctionSections(false),

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

I will land this later today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145715

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

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Mentioned removal of flag in release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145715

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

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

[Julian Lettner via Phabricator via cfe-commits]

yln updated this revision to Diff 504890.
yln marked an inline comment as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145715

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll

Index: llvm/test/CodeGen/ARM/ctors_dtors.ll
===
--- llvm/test/CodeGen/ARM/ctors_dtors.ll
+++ llvm/test/CodeGen/ARM/ctors_dtors.ll
@@ -1,5 +1,4 @@
 ; RUN: llc < %s -mtriple=arm-apple-darwin  | FileCheck %s -check-prefix=DARWIN
-; RUN: llc < %s -mtriple=arm-apple-darwin -lower-global-dtors-via-cxa-atexit=false  | FileCheck %s -check-prefix=DARWIN-OLD
 ; RUN: llc < %s -mtriple=arm-linux-gnu -target-abi=apcs  | FileCheck %s -check-prefix=ELF
 ; RUN: llc < %s -mtriple=arm-linux-gnueabi | FileCheck %s -check-prefix=GNUEABI
 
@@ -8,9 +7,6 @@
 ; DARWIN: .section	__DATA,__mod_init_func,mod_init_funcs
 ; DARWIN-NOT: __mod_term_func
 
-; DARWIN-OLD: .section	__DATA,__mod_init_func,mod_init_funcs
-; DARWIN-OLD: .section	__DATA,__mod_term_func,mod_term_funcs
-
 ; ELF: .section .ctors,"aw",%progbits
 ; ELF: .section .dtors,"aw",%progbits
 
Index: llvm/lib/CodeGen/TargetPassConfig.cpp
===
--- llvm/lib/CodeGen/TargetPassConfig.cpp
+++ llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -878,8 +878,7 @@
 
   // For MachO, lower @llvm.global_dtors into @llvm.global_ctors with
   // __cxa_atexit() calls to avoid emitting the deprecated __mod_term_func.
-  if (TM->getTargetTriple().isOSBinFormatMachO() &&
-  TM->Options.LowerGlobalDtorsViaCxaAtExit)
+  if (TM->getTargetTriple().isOSBinFormatMachO())
 addPass(createLowerGlobalDtorsLegacyPass());
 
   // Make sure that no unreachable blocks are instruction selected.
Index: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
===
--- llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -1202,11 +1202,7 @@
 
 MCSection *TargetLoweringObjectFileMachO::getStaticDtorSection(
 unsigned Priority, const MCSymbol *KeySym) const {
-  // TODO(yln): Remove -lower-global-dtors-via-cxa-atexit fallback flag
-  // (LowerGlobalDtorsViaCxaAtExit) and always issue a fatal error here.
-  if (TM->Options.LowerGlobalDtorsViaCxaAtExit)
-report_fatal_error("@llvm.global_dtors should have been lowered already");
-  return StaticDtorSection;
+  report_fatal_error("@llvm.global_dtors should have been lowered already");
 }
 
 void TargetLoweringObjectFileMachO::emitModuleMetadata(MCStreamer ,
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,7 +81,6 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
-CGOPT(bool, LowerGlobalDtorsViaCxaAtExit)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -349,12 +348,6 @@
 cl::init(false));
   CGBINDOPT(UseCtors);
 
-  static cl::opt LowerGlobalDtorsViaCxaAtExit(
-  "lower-global-dtors-via-cxa-atexit",
-  cl::desc("Lower llvm.global_dtors (global destructors) via __cxa_atexit"),
-  cl::init(true));
-  CGBINDOPT(LowerGlobalDtorsViaCxaAtExit);
-
   static cl::opt RelaxELFRelocations(
   "relax-elf-relocations",
   cl::desc(
@@ -538,7 +531,6 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
-  Options.LowerGlobalDtorsViaCxaAtExit = getLowerGlobalDtorsViaCxaAtExit();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -130,7 +130,7 @@
   HonorSignDependentRoundingFPMathOption(false), NoZerosInBSS(false),
   GuaranteedTailCallOpt(false), StackSymbolOrdering(true),
   EnableFastISel(false), EnableGlobalISel(false), UseInitArray(false),
-  LowerGlobalDtorsViaCxaAtExit(false), DisableIntegratedAS(false),
+  DisableIntegratedAS(false),
   RelaxELFRelocations(true), FunctionSections(false),
   DataSections(false), IgnoreXCOFFVisibility(false),
   XCOFFTracebackTable(true), UniqueSectionNames(true),
@@ -247,10 +247,6 @@
   

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit flag

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1205
 unsigned Priority, const MCSymbol *KeySym) const {
-  // TODO(yln): Remove -lower-global-dtors-via-cxa-atexit fallback flag
-  // (LowerGlobalDtorsViaCxaAtExit) and always issue a fatal error here.
-  if (TM->Options.LowerGlobalDtorsViaCxaAtExit)
-report_fatal_error("@llvm.global_dtors should have been lowered already");
-  return StaticDtorSection;
+  report_fatal_error("@llvm.global_dtors should have been lowered already");
 }

rsundahl wrote:
> So the new assertion will be that it's a fatal error to get here at all. 
> Currently, it's a fatal error only if you get here with the flag set. Are we 
> sure that someone isn't getting here w/o the flag set and so they don't get 
> an error but now will? Provided we don't mind surfacing any users of the flag 
> with an explicit fatal error, then this LGTM.
> Currently, it's a fatal error only if you get here with the flag set. Are we 
> sure that someone isn't getting here w/o the flag set and so they don't get 
> an error but now will?

In the previous revision, I switched the default behavior for destructor 
lowering for MachO, but added this flag as an escape hatch to request the old 
behavior.  The switch in behavior is encoded as follows.  Note: the default of 
this flag (and resulting `Options.LowerGlobalDtorsViaCxaAtExit`) is true.

```
  if (TM->getTargetTriple().isOSBinFormatMachO() && 
TM->Options.LowerGlobalDtorsViaCxaAtExit)
addPass(createLowerGlobalDtorsLegacyPass())
```

So the intention was/is that no end user should/will be able to reach this 
fatal error, it's more of an internal assert for us.

Previously, one could have supplied `-lower-global-dtors-via-cxa-atexit=false` 
to set `LowerGlobalDtorsViaCxaAtExit` to false. I am not aware that anyone ever 
used the flag / escape hatch.

The error that users get if they had previously supplied the flag will be the 
following after this change:
```
Unknown command line argument '-lower-global-dtors-via-cxa-atexit=false'.
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145715

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

[PATCH] D145715: Remove -lower-global-dtors-via-cxa-atexit

[Julian Lettner via Phabricator via cfe-commits]

yln created this revision.
Herald added subscribers: ormris, hiraditya.
Herald added a project: All.
yln requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Remove the `-lower-global-dtors-via-cxa-atexit` escape hatch introduced
in D121736  [1], which switched the default 
lowering of global
destructors on MachO to use `__cxa_atexit()` to avoid emitting
deprecated `__mod_term_func` sections.

I added this flag as an escape hatch in case the switch causes any
problems.  We didn't discover any problems so now we can remove it.

[1] https://reviews.llvm.org/D121736

rdar://90277838


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145715

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll

Index: llvm/test/CodeGen/ARM/ctors_dtors.ll
===
--- llvm/test/CodeGen/ARM/ctors_dtors.ll
+++ llvm/test/CodeGen/ARM/ctors_dtors.ll
@@ -1,5 +1,4 @@
 ; RUN: llc < %s -mtriple=arm-apple-darwin  | FileCheck %s -check-prefix=DARWIN
-; RUN: llc < %s -mtriple=arm-apple-darwin -lower-global-dtors-via-cxa-atexit=false  | FileCheck %s -check-prefix=DARWIN-OLD
 ; RUN: llc < %s -mtriple=arm-linux-gnu -target-abi=apcs  | FileCheck %s -check-prefix=ELF
 ; RUN: llc < %s -mtriple=arm-linux-gnueabi | FileCheck %s -check-prefix=GNUEABI
 
@@ -8,9 +7,6 @@
 ; DARWIN: .section	__DATA,__mod_init_func,mod_init_funcs
 ; DARWIN-NOT: __mod_term_func
 
-; DARWIN-OLD: .section	__DATA,__mod_init_func,mod_init_funcs
-; DARWIN-OLD: .section	__DATA,__mod_term_func,mod_term_funcs
-
 ; ELF: .section .ctors,"aw",%progbits
 ; ELF: .section .dtors,"aw",%progbits
 
Index: llvm/lib/CodeGen/TargetPassConfig.cpp
===
--- llvm/lib/CodeGen/TargetPassConfig.cpp
+++ llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -878,8 +878,7 @@
 
   // For MachO, lower @llvm.global_dtors into @llvm.global_ctors with
   // __cxa_atexit() calls to avoid emitting the deprecated __mod_term_func.
-  if (TM->getTargetTriple().isOSBinFormatMachO() &&
-  TM->Options.LowerGlobalDtorsViaCxaAtExit)
+  if (TM->getTargetTriple().isOSBinFormatMachO())
 addPass(createLowerGlobalDtorsLegacyPass());
 
   // Make sure that no unreachable blocks are instruction selected.
Index: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
===
--- llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -1202,11 +1202,7 @@
 
 MCSection *TargetLoweringObjectFileMachO::getStaticDtorSection(
 unsigned Priority, const MCSymbol *KeySym) const {
-  // TODO(yln): Remove -lower-global-dtors-via-cxa-atexit fallback flag
-  // (LowerGlobalDtorsViaCxaAtExit) and always issue a fatal error here.
-  if (TM->Options.LowerGlobalDtorsViaCxaAtExit)
-report_fatal_error("@llvm.global_dtors should have been lowered already");
-  return StaticDtorSection;
+  report_fatal_error("@llvm.global_dtors should have been lowered already");
 }
 
 void TargetLoweringObjectFileMachO::emitModuleMetadata(MCStreamer ,
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,7 +81,6 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
-CGOPT(bool, LowerGlobalDtorsViaCxaAtExit)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -349,12 +348,6 @@
 cl::init(false));
   CGBINDOPT(UseCtors);
 
-  static cl::opt LowerGlobalDtorsViaCxaAtExit(
-  "lower-global-dtors-via-cxa-atexit",
-  cl::desc("Lower llvm.global_dtors (global destructors) via __cxa_atexit"),
-  cl::init(true));
-  CGBINDOPT(LowerGlobalDtorsViaCxaAtExit);
-
   static cl::opt RelaxELFRelocations(
   "relax-elf-relocations",
   cl::desc(
@@ -538,7 +531,6 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
-  Options.LowerGlobalDtorsViaCxaAtExit = getLowerGlobalDtorsViaCxaAtExit();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -130,7 +130,7 @@
   

[PATCH] D144672: [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin

[Julian Lettner via Phabricator via cfe-commits]

yln accepted this revision.
yln added inline comments.



Comment at: 
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11
+
+// darwin only supports shared-libsan, so this should fail.
+// XFAIL: darwin
+ 

usama54321 wrote:
> yln wrote:
> > usama54321 wrote:
> > > yln wrote:
> > > > zixuw wrote:
> > > > > dmaclach wrote:
> > > > > > yln wrote:
> > > > > > > This should work, right?
> > > > > > No.. darwin should fail with the `-static-libsan` flag. This is the 
> > > > > > test that was failing and caused the rollback.
> > > > > I think @yln is suggesting using `REQUIRES: asan-static-runtime` 
> > > > > instead of `XFAIL: darwin`. I wasn't aware of that conditional but 
> > > > > yeah that should be better if it works.
> > > > I meant using `// REQUIRES: asan-static-runtime ` instead of `XFAIL: 
> > > > darwin` since it seems that we already have a lit feature for it.
> > > I think UNSUPPORTED: darwin makes the most sense here. I don't think lit 
> > > understands that REQUIRES: asan-static-runtime should result in skipping 
> > > the test on Darwin as it does not know about this dependency.
> > > I don't think lit understands that REQUIRES: asan-static-runtime should 
> > > result in skipping the test on Darwin as it does not know about this 
> > > dependency.
> > 
> > Actually, this was exactly my point.  We have other tests already marked 
> > with `REQUIRES: asan-static-runtime` and we should double check our changes 
> > don't affect these as well.
> > 
> > If LIT doesn't model this dependency yet, then we should make sure it does! 
> >  And this test can act as a good "canary in the coal mine".
> > 
> > Please use `REQUIRES: asan-static-runtime` and make sure we understand and 
> > deal with any fallout.
> Sorry for my earlier comment. @yln is correct, and we should use REQUIRES: 
> asan-static-runtime. I double checked, and this already works as expected on 
> darwin, i.e. these tests are unsupported on darwin. So you should not need to 
> do anything apart from adding the REQUIRES in the test.
> [...] I double checked, and this already works as expected on darwin, i.e. 
> these tests are unsupported on darwin.

Thanks Usama!

Patch LGTM assuming we switch to using `REQUIRES: asan-static-runtime`.  Thanks 
for seeing this through! :)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144672

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

[PATCH] D144672: [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: 
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11
+
+// darwin only supports shared-libsan, so this should fail.
+// XFAIL: darwin
+ 

usama54321 wrote:
> yln wrote:
> > zixuw wrote:
> > > dmaclach wrote:
> > > > yln wrote:
> > > > > This should work, right?
> > > > No.. darwin should fail with the `-static-libsan` flag. This is the 
> > > > test that was failing and caused the rollback.
> > > I think @yln is suggesting using `REQUIRES: asan-static-runtime` instead 
> > > of `XFAIL: darwin`. I wasn't aware of that conditional but yeah that 
> > > should be better if it works.
> > I meant using `// REQUIRES: asan-static-runtime ` instead of `XFAIL: 
> > darwin` since it seems that we already have a lit feature for it.
> I think UNSUPPORTED: darwin makes the most sense here. I don't think lit 
> understands that REQUIRES: asan-static-runtime should result in skipping the 
> test on Darwin as it does not know about this dependency.
> I don't think lit understands that REQUIRES: asan-static-runtime should 
> result in skipping the test on Darwin as it does not know about this 
> dependency.

Actually, this was exactly my point.  We have other tests already marked with 
`REQUIRES: asan-static-runtime` and we should double check our changes don't 
affect these as well.

If LIT doesn't model this dependency yet, then we should make sure it does!  
And this test can act as a good "canary in the coal mine".

Please use `REQUIRES: asan-static-runtime` and make sure we understand and deal 
with any fallout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144672

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

[PATCH] D144672: [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: 
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11
+
+// darwin only supports shared-libsan, so this should fail.
+// XFAIL: darwin
+ 

zixuw wrote:
> dmaclach wrote:
> > yln wrote:
> > > This should work, right?
> > No.. darwin should fail with the `-static-libsan` flag. This is the test 
> > that was failing and caused the rollback.
> I think @yln is suggesting using `REQUIRES: asan-static-runtime` instead of 
> `XFAIL: darwin`. I wasn't aware of that conditional but yeah that should be 
> better if it works.
I meant using `// REQUIRES: asan-static-runtime ` instead of `XFAIL: darwin` 
since it seems that we already have a lit feature for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144672

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

[PATCH] D144672: [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: 
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp:10-11
+
+// darwin only supports shared-libsan, so this should fail.
+// XFAIL: darwin
+ 

This should work, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144672

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

[PATCH] D142421: [Sanitizers] fix -fno-sanitize-link-runtime for darwin

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Looks harmless enough, even though I am not sure why we have a completely 
separate code path on Darwin.  The other toolchain driver seem to have shared 
code for this in `clang/lib/Driver/ToolChains/CommonArgs.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142421

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

[PATCH] D139652: Add the thread sanitizer support for X86_64 WatchOS simulators

[Julian Lettner via Phabricator via cfe-commits]

yln accepted this revision.
yln added a subscriber: rsundahl.
yln added a comment.

Thank you! LGTM, with one nit.

@rsundahl: can you keep an eye on this and resolve potential conflicts?  Thanks!




Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:3280-3286
   if ((IsX86_64 || IsAArch64) && isTargetMacOSBased()) {
 Res |= SanitizerKind::Thread;
-  } else if (isTargetIOSSimulator() || isTargetTvOSSimulator()) {
+  } else if (isTargetIOSSimulator() || isTargetTvOSSimulator() ||
+ isTargetWatchOSSimulator()) {
 if (IsX86_64)
   Res |= SanitizerKind::Thread;
   }

Yes, for watchOS simulator.  I think we can also drop the constraint on x86_64 
and then merge the if.  TSan //should// be supported for simulators on Apple 
Silicon.  Can you add additional tests for these 3 cases?  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139652

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

[PATCH] D137484: [Test] Fix driverkit-path.c with lib64 dir

[Julian Lettner via Phabricator via cfe-commits]

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

Looks harmless enough.. LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137484

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

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2349
+  if (!cookieOffset.isZero())
+cookiePtr = CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, 
cookieOffset);
 

Variable names should start with uppercase:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Please also be conservative with "rename cleanups" in reviews.  The smaller 
your patch is, the easier it is to review.  You can do these in a follow-up NFC 
commit.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:266
+#endif
 }
 

I just realized that we are poisoning the cookie, but we are not doing anything 
on the detection side.
Is this actually required to make detection work/the tests pass?

If this bit isn't needed to make the existing tests pass, then I would like to 
suggest 1 of 2 things:
* Remove the additional poisoning (recommended), -or-
* Add another (ARM-specific?) test that requires poisoning of the second cookie 
word.  I think this would require adaptation of `__asan_load_cxx_array_cookie` 
as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195

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

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:263
+#if SANITIZER_ARM64
+  // The ARM64 cookie has a second "size_t" entry so poison it as well
+  *(reinterpret_cast(s)-1) = kAsanArrayCookieMagic;





Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:19
 int main(int argc, char **argv) {
-  C *buffer = new C[argc];
-  buffer[-2].x = 10;
+  C *buffer = new C[1];
+  reinterpret_cast(buffer)[-1] = 42;

`C *buffer = new C[argc];`

Although this looks weird, I think this is to prevent the compiler from 
reasoning about the array size.
I know that we had tests in the past that were "silently passing", because the 
compiler optimized away parts of the test.

I am not sure if it applies here, but let's keep it just in case ...



Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:20
+  C *buffer = new C[1];
+  reinterpret_cast(buffer)[-1] = 42;
 // CHECK: AddressSanitizer: heap-buffer-overflow





Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:4-15
+// Here we test that:
+//   1) w/o ASan, the array cookie location IS writable
+//   2) w/ASan, the array cookie location IS writeable by default
+//   3) w/ASan, the flag "-fno-sanitize-address-poison-custom-array-cookie"
+//  is a NOP (because this is the default) and finally,
+//   4) w/ASan AND "-fsanitize-address-poison-custom-array-cookie", the
+//  array cookie location is NOT writable (ASAN successfully stopped it)

Thanks for improving the test like this!  This is now better than "just a 
test", it's essentially "executable documentation"! :)



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:36-38
-  assert(reinterpret_cast(foo) ==
- reinterpret_cast(Foo::allocated) + sizeof(void*));
-  *reinterpret_cast(Foo::allocated) = 42;

Is the reason that we had to remove this assert (and change how we overwrite 
the cookie) that on arm the cookie is 2 words?

Can we do the following?
```
int cooke_words =  : 2 : 1;
assert(reinterpret_cast(foo) ==
 reinterpret_cast(Foo::allocated) + cookie_words * 
sizeof(void*));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195

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

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262
   *reinterpret_cast(s) = kAsanArrayCookieMagic;
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64

yln wrote:
> yln wrote:
> > Nitpicking extreme:
> > * I think the code inside `#if` shouldn't have extra indent (because 
> > preprocessor sections don't establish code).  If in doubt, let 
> > `clang-format` do it for you.
> > * Let's move the comment inside the #if, just above before the line of 
> > code.  If you ever read the pre-processed source-code, then the comment 
> > "lives and dies" with the line of code it relates too, i.e, on x86, 
> > currently there would be a comment without the code.
> 
I find this a bit confusing
* x86_64: cookie is 1 word and passed `p` points to it
* arm64: cookie is 2 words and passed `p` points to second half of it

Would it be worth to take the extra care in CodeGen to always pass the 
"beginning of the cookie" to `__asan_poison_cxx_array_cookie()` and then have 
something like that:
```
size_t shadow_cookie_size = SANITIZER_ARM64 ? 2 : 1:
internal_memset(s, kAsanArrayCookieMagic, shadow_cookie_size);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195

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

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262
   *reinterpret_cast(s) = kAsanArrayCookieMagic;
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64

yln wrote:
> Nitpicking extreme:
> * I think the code inside `#if` shouldn't have extra indent (because 
> preprocessor sections don't establish code).  If in doubt, let `clang-format` 
> do it for you.
> * Let's move the comment inside the #if, just above before the line of code.  
> If you ever read the pre-processed source-code, then the comment "lives and 
> dies" with the line of code it relates too, i.e, on x86, currently there 
> would be a comment without the code.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195

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

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2453
+CGF.Builder.CreateCall(F, cookie.getRawPointer(CGF));
+  }
 

This code is very similar to what we have in 
`ItaniumCXXABI::InitializeArrayCookie()`.  Can we try to extract a local 
`handleASanArrayCookiePoisoning()` helper?



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2485
+  CGM.CreateRuntimeFunction(FTy, "__asan_load_cxx_array_cookie");
+  return CGF.Builder.CreateCall(F, numElementsPtr.getRawPointer(CGF));
 }

Same here: extract common helper for the ASan stuff to be shared by ARM and 
ItaniumCXXABI.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262-265
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64
+*(reinterpret_cast(s)-1) = kAsanArrayCookieMagic;
+  #endif

Nitpicking extreme:
* I think the code inside `#if` shouldn't have extra indent (because 
preprocessor sections don't establish code).  If in doubt, let `clang-format` 
do it for you.
* Let's move the comment inside the #if, just above before the line of code.  
If you ever read the pre-processed source-code, then the comment "lives and 
dies" with the line of code it relates too, i.e, on x86, currently there would 
be a comment without the code.



Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:3
 // RUN: %clangxx_asan -O3 %s -o %t
-// RUN:not %run %t 2>&1  | FileCheck %s
+// RUN:  not %run %t 2>&1  | FileCheck %s
 // RUN: %env_asan_opts=poison_array_cookie=1 not %run %t 2>&1  | FileCheck %s

❤  The boy scouts rule:
> Always leave the campground cleaner than you found it.



Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:19
   C *buffer = new C[argc];
-  buffer[-2].x = 10;
+  buffer[-1].x = 10;
 // CHECK: AddressSanitizer: heap-buffer-overflow

Can we try to simulate a 1-byte buffer-underflow (and leave the definition of 
`struct C` as-is)?  This would show that ASan still reports the smallest 
possible infraction.



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp:20
 __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) {
-  long *p = reinterpret_cast(c);
+  size_t *p = reinterpret_cast(c);
   p[-1] = 42;

Can we try to simulate a 1-byte buffer-underflow (and leave the definition of 
struct C as-is)? This would show that ASan still reports the smallest possible 
infraction.



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:5
+// Here we test that:
+//   1) w/o sanitizer, the array cookie location IS writable
+//   2) w/sanitizer, the array cookie location IS writeable by default

sanitizer -> ASan



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:12
 //
-// XFAIL: arm
-
-// UNSUPPORTED: ios
+// RUN: %clangxx   %s -o 
%t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
+// RUN: %clangxx_asan  %s -o 
%t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT

Avoid `NOT` in FileCheck prefix names, because `-NOT` is the directive to do a 
"negative match", that is, "make sure this text does not appear".

How about:
`--check-prefix=COOKIE` for the cookie case and just skip `--check-prefix` for 
the "no cookie" (it's the default after all)



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:35
-  fprintf(stderr, "foo  : %p\n", foo);
-  fprintf(stderr, "alloc: %p\n", Foo::allocated);
-  assert(reinterpret_cast(foo) ==

Why remove this debug output?  It might be useful...



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:43-47
+// SANITIZE_YES: AddressSanitizer: heap-buffer-overflow
+// SANITIZE_YES: in main 
{{.*}}new_array_cookie_with_new_from_class.cpp:[[@LINE-2]]
+// SANITIZE_YES: is located 0 bytes inside of {{18|26}}-byte region
+  printf("Unsurprisingly, we were able to write to the array cookie\n");
+// SANITIZE_NOT: Unsurprisingly, we were able to write to the array cookie




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195

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

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Thank you @zequanwu, for providing the minimized reproducer! It really made 
fixing this much easier!  :)

I've re-landed the change and added a test specifically for this issue: 
`lower-global-dtors-existing-dos_handle.ll`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121736

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

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

In D121736#3389251 , @RKSimon wrote:

> LGTM - but please hang around after you commit

Thanks!  Looks like this it worked out.  Let me know if there is any other 
problems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121736

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

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner 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 rG22570bac6943: Lower `@llvm.global_dtors` using 
`__cxa_atexit` on MachO (authored by yln).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121736

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/docs/Passes.rst
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/LinkAllPasses.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h
  llvm/include/llvm/Transforms/Utils.h
  llvm/include/llvm/Transforms/Utils/LowerGlobalDtors.h
  llvm/lib/CodeGen/CodeGen.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Target/WebAssembly/CMakeLists.txt
  llvm/lib/Target/WebAssembly/WebAssembly.h
  llvm/lib/Target/WebAssembly/WebAssemblyLowerGlobalDtors.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/LowerGlobalDtors.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll
  llvm/test/CodeGen/X86/2011-08-29-InitOrder.ll
  llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll

Index: llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
@@ -0,0 +1,156 @@
+; RUN: opt-lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+; RUN: opt -passes=lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+
+; Test that @llvm.global_dtors is properly lowered into @llvm.global_ctors,
+; grouping dtor calls by priority and associated symbol.
+
+declare void @orig_ctor()
+declare void @orig_dtor0()
+declare void @orig_dtor1a()
+declare void @orig_dtor1b()
+declare void @orig_dtor1c0()
+declare void @orig_dtor1c1a()
+declare void @orig_dtor1c1b()
+declare void @orig_dtor1c2a()
+declare void @orig_dtor1c2b()
+declare void @orig_dtor1c3()
+declare void @orig_dtor1d()
+declare void @orig_dtor65535()
+declare void @orig_dtor65535c0()
+declare void @after_the_null()
+
+@associatedc0 = external global i8
+@associatedc1 = external global i8
+@associatedc2 = global i8 42
+@associatedc3 = global i8 84
+
+@llvm.global_ctors = appending global
+[1 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null }
+]
+
+@llvm.global_dtors = appending global
+[14 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 0, void ()* @orig_dtor0, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1a, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1b, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1a, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1b, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2a, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2b, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c3, i8* @associatedc3 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1d, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* null, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @after_the_null, i8* null }
+]
+
+; CHECK: @associatedc0 = external global i8
+; CHECK: @associatedc1 = external global i8
+; CHECK: @associatedc2 = global i8 42
+; CHECK: @associatedc3 = global i8 84
+; CHECK: @__dso_handle = extern_weak hidden constant i8
+
+; CHECK-LABEL: @llvm.global_ctors = appending global [10 x { i32, void ()*, i8* }] [
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 0, void ()* @register_call_dtors.0, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$0", i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$1.associatedc0", i8* @associatedc0 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$2.associatedc1", i8* @associatedc1 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$3.associatedc2", i8* @associatedc2 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* 

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Plan to land tomorrow morning.  Test passes on the Debian bot 
:

  PASS: LLVM :: Transforms/LowerGlobalDestructors/lower-global-dtors.ll (72299 
of 93291)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121736

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

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

I've added 2 additional calls to `initializeLowerGlobalDtorsLegacyPassPass()`.  
The one in the constructor of the legacy pass should ensure that the pass is 
always registered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121736

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

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

It looks like all tests succeed for `x64 debian`:
https://buildkite.com/llvm-project/premerge-checks/builds/83802#c5002be5-e01c-46fc-a767-50fe6f854465


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121736

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

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Hi @Orlando and @RKSimon, would you be able to confirm that this version avoids 
the previous `opt: Unknown command line argument '-lower-global-dtors'` error?

Lit invocation:
`env LIT_FILTER='dtor' ninja check-llvm`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121736

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

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

New revision here: D121736 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

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

[PATCH] D121736: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln created this revision.
yln added reviewers: Orlando, RKSimon.
Herald added subscribers: asb, ormris, pengfei, sunfish, hiraditya, 
jgravelle-google, sbc100, mgorny, dschuff.
Herald added a project: All.
yln requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, aheejin.
Herald added projects: clang, LLVM.

For MachO, lower `@llvm.global_dtors` into `@llvm_global_ctors` with
`__cxa_atexit` calls to avoid emitting the deprecated `__mod_term_func`.

Reuse the existing `WebAssemblyLowerGlobalDtors.cpp` to accomplish this.

Enable fallback to the old behavior via Clang driver flag
(`-fregister-global-dtors-with-atexit`) or llc / code generation flag
(`-lower-global-dtors-via-cxa-atexit`).  This escape hatch will be
removed in the future.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121736

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/docs/Passes.rst
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/LinkAllPasses.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h
  llvm/include/llvm/Transforms/Utils.h
  llvm/include/llvm/Transforms/Utils/LowerGlobalDtors.h
  llvm/lib/CodeGen/CodeGen.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Target/WebAssembly/CMakeLists.txt
  llvm/lib/Target/WebAssembly/WebAssembly.h
  llvm/lib/Target/WebAssembly/WebAssemblyLowerGlobalDtors.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/LowerGlobalDtors.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll
  llvm/test/CodeGen/X86/2011-08-29-InitOrder.ll
  llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll

Index: llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
@@ -0,0 +1,156 @@
+; RUN: opt-lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+; RUN: opt -passes=lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+
+; Test that @llvm.global_dtors is properly lowered into @llvm.global_ctors,
+; grouping dtor calls by priority and associated symbol.
+
+declare void @orig_ctor()
+declare void @orig_dtor0()
+declare void @orig_dtor1a()
+declare void @orig_dtor1b()
+declare void @orig_dtor1c0()
+declare void @orig_dtor1c1a()
+declare void @orig_dtor1c1b()
+declare void @orig_dtor1c2a()
+declare void @orig_dtor1c2b()
+declare void @orig_dtor1c3()
+declare void @orig_dtor1d()
+declare void @orig_dtor65535()
+declare void @orig_dtor65535c0()
+declare void @after_the_null()
+
+@associatedc0 = external global i8
+@associatedc1 = external global i8
+@associatedc2 = global i8 42
+@associatedc3 = global i8 84
+
+@llvm.global_ctors = appending global
+[1 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null }
+]
+
+@llvm.global_dtors = appending global
+[14 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 0, void ()* @orig_dtor0, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1a, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1b, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1a, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1b, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2a, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2b, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c3, i8* @associatedc3 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1d, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* null, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @after_the_null, i8* null }
+]
+
+; CHECK: @associatedc0 = external global i8
+; CHECK: @associatedc1 = external global i8
+; CHECK: @associatedc2 = global i8 42
+; CHECK: @associatedc3 = global i8 84
+; CHECK: @__dso_handle = extern_weak hidden constant i8
+
+; CHECK-LABEL: @llvm.global_ctors = appending global [10 x { i32, void ()*, i8* }] [
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 0, void ()* @register_call_dtors.0, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } 

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Hi @Orlando and @RKSimon!

Thanks for pointing out the test failure and reverting the change.  I see this 
failure:

  : 'RUN: at line 1';   
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/opt
-lower-global-dtors -S < 
/home/buildbot/buildbot-root/llvm-project/llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
 | 
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/FileCheck
 
/home/buildbot/buildbot-root/llvm-project/llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
 --implicit-check-not=llvm.global_dtors
  : 'RUN: at line 2';   
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/opt 
-passes=lower-global-dtors -S < 
/home/buildbot/buildbot-root/llvm-project/llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
 | 
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/FileCheck
 
/home/buildbot/buildbot-root/llvm-project/llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
 --implicit-check-not=llvm.global_dtors
  --
  Exit Code: 2
  Command Output (stderr):
  --
  opt: Unknown command line argument '-lower-global-dtors'.  Try: 
'/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/opt 
--help'
  opt: Did you mean '--join-globalcopies'?

This looks like my changes are not sufficiently registering the pass for `opt` 
on every platform, but so far I haven't found anything that is obviously 
wrong/missing.  Do you know who I can reach out to help me with "pass 
registration"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

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

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
yln marked an inline comment as done.
Closed by commit rG9c542a5a4e1b: Lower `@llvm.global_dtors` using 
`__cxa_atexit` on MachO (authored by yln).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/docs/Passes.rst
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/LinkAllPasses.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h
  llvm/include/llvm/Transforms/Utils.h
  llvm/include/llvm/Transforms/Utils/LowerGlobalDtors.h
  llvm/lib/CodeGen/CodeGen.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Target/WebAssembly/CMakeLists.txt
  llvm/lib/Target/WebAssembly/WebAssembly.h
  llvm/lib/Target/WebAssembly/WebAssemblyLowerGlobalDtors.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/LowerGlobalDtors.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll
  llvm/test/CodeGen/X86/2011-08-29-InitOrder.ll
  llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll

Index: llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
@@ -0,0 +1,156 @@
+; RUN: opt-lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+; RUN: opt -passes=lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+
+; Test that @llvm.global_dtors is properly lowered into @llvm.global_ctors,
+; grouping dtor calls by priority and associated symbol.
+
+declare void @orig_ctor()
+declare void @orig_dtor0()
+declare void @orig_dtor1a()
+declare void @orig_dtor1b()
+declare void @orig_dtor1c0()
+declare void @orig_dtor1c1a()
+declare void @orig_dtor1c1b()
+declare void @orig_dtor1c2a()
+declare void @orig_dtor1c2b()
+declare void @orig_dtor1c3()
+declare void @orig_dtor1d()
+declare void @orig_dtor65535()
+declare void @orig_dtor65535c0()
+declare void @after_the_null()
+
+@associatedc0 = external global i8
+@associatedc1 = external global i8
+@associatedc2 = global i8 42
+@associatedc3 = global i8 84
+
+@llvm.global_ctors = appending global
+[1 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null }
+]
+
+@llvm.global_dtors = appending global
+[14 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 0, void ()* @orig_dtor0, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1a, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1b, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1a, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1b, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2a, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2b, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c3, i8* @associatedc3 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1d, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* null, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @after_the_null, i8* null }
+]
+
+; CHECK: @associatedc0 = external global i8
+; CHECK: @associatedc1 = external global i8
+; CHECK: @associatedc2 = global i8 42
+; CHECK: @associatedc3 = global i8 84
+; CHECK: @__dso_handle = extern_weak hidden constant i8
+
+; CHECK-LABEL: @llvm.global_ctors = appending global [10 x { i32, void ()*, i8* }] [
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 0, void ()* @register_call_dtors.0, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$0", i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$1.associatedc0", i8* @associatedc0 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$2.associatedc1", i8* @associatedc1 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$3.associatedc2", i8* @associatedc2 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* 

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

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



Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:900
+  // __cxa_atexit calls to avoid emitting the deprecated __mod_term_func.
+  if (TM->getTargetTriple().isOSBinFormatMachO())
+addPass(createLowerGlobalDtorsLegacyPass());

delcypher wrote:
> yln wrote:
> > yln wrote:
> > > delcypher wrote:
> > > > Random thought. Do we want to support the legacy way of calling 
> > > > destructors, rather than removing it entirely? If we were to do such a 
> > > > thing I'd suspect we'd guard using the legacy way on the OS deployment 
> > > > target.
> > > > 
> > > > Just to be clear. I'm happy with the patch the way it is. I'm just 
> > > > wondering if we should consider allowing the legacy way as well. I 
> > > > can't see an obvious use case for it because the new way should work on 
> > > > older OSs too but maybe there's a use case I haven't thought about?
> > > Having a way to explicitly request the old behavior sounds like a good 
> > > idea.  I will look into it.
> > I added an escape hatch to fallback to the old behavior:
> > * via Clang driver flag `-fregister-global-dtors-with-atexit`
> > * llc / code generation flag -lower-global-dtors-via-cxa-atexit.
> > This escape hatch will be removed in the future.
> @yln I don't see any modifications to the driver. How is 
> `-fregister-global-dtors-with-atexit` added as a driver flag?
This is an existing flag in the frontend added as part of this change: D45578
```
Add a command line option `fregister_global_dtors_with_atexit` to register 
destructor functions annotated with __attribute__((destructor)) using 
__cxa_atexit or atexit.
```

It's on by default on Darwin:
```
  if (Args.hasFlag(options::OPT_fregister_global_dtors_with_atexit,
   options::OPT_fno_register_global_dtors_with_atexit,
   RawTriple.isOSDarwin() && !KernelOrKext))
CmdArgs.push_back("-fregister-global-dtors-with-atexit");
```

Essentially we switched over the frontend to avoid emitting the deprecated 
`__mod_term_func` section a long time ago and now I am using the same technique 
to lower `.llvm.global_dtors` in the backend.  So we could say the flag is 
exactly what we wanted to express (it makes sense to key off it), but now it 
applies more fully.

Before D45578, destructor functions annotated with 
`__attribute__((destructor))` were added to `.llvm.global_dtor`.  I do not know 
why we decided to lower them in the frontend (where it didn't apply to 
`.llvm.global_dtors`) instead of the backend (this change).

There is certainly an opportunity here to refactor this and make the frontend 
use `llvm.global_dtor` again to reduce code duplication (after making sure we 
cover all the necessary cases).  That's a big and wide-reaching change and  I 
am not intending to do this as part of this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

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

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln updated this revision to Diff 415282.
yln added a comment.

Report fatal error when we can, instead of never.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/docs/Passes.rst
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/LinkAllPasses.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h
  llvm/include/llvm/Transforms/Utils.h
  llvm/include/llvm/Transforms/Utils/LowerGlobalDtors.h
  llvm/lib/CodeGen/CodeGen.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Target/WebAssembly/CMakeLists.txt
  llvm/lib/Target/WebAssembly/WebAssembly.h
  llvm/lib/Target/WebAssembly/WebAssemblyLowerGlobalDtors.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/LowerGlobalDtors.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll
  llvm/test/CodeGen/X86/2011-08-29-InitOrder.ll
  llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll

Index: llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
@@ -0,0 +1,156 @@
+; RUN: opt-lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+; RUN: opt -passes=lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+
+; Test that @llvm.global_dtors is properly lowered into @llvm.global_ctors,
+; grouping dtor calls by priority and associated symbol.
+
+declare void @orig_ctor()
+declare void @orig_dtor0()
+declare void @orig_dtor1a()
+declare void @orig_dtor1b()
+declare void @orig_dtor1c0()
+declare void @orig_dtor1c1a()
+declare void @orig_dtor1c1b()
+declare void @orig_dtor1c2a()
+declare void @orig_dtor1c2b()
+declare void @orig_dtor1c3()
+declare void @orig_dtor1d()
+declare void @orig_dtor65535()
+declare void @orig_dtor65535c0()
+declare void @after_the_null()
+
+@associatedc0 = external global i8
+@associatedc1 = external global i8
+@associatedc2 = global i8 42
+@associatedc3 = global i8 84
+
+@llvm.global_ctors = appending global
+[1 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null }
+]
+
+@llvm.global_dtors = appending global
+[14 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 0, void ()* @orig_dtor0, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1a, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1b, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1a, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1b, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2a, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2b, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c3, i8* @associatedc3 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1d, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* null, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @after_the_null, i8* null }
+]
+
+; CHECK: @associatedc0 = external global i8
+; CHECK: @associatedc1 = external global i8
+; CHECK: @associatedc2 = global i8 42
+; CHECK: @associatedc3 = global i8 84
+; CHECK: @__dso_handle = extern_weak hidden constant i8
+
+; CHECK-LABEL: @llvm.global_ctors = appending global [10 x { i32, void ()*, i8* }] [
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 0, void ()* @register_call_dtors.0, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$0", i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$1.associatedc0", i8* @associatedc0 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$2.associatedc1", i8* @associatedc1 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$3.associatedc2", i8* @associatedc2 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$4.associatedc3", i8* @associatedc3 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$5", i8* null },
+; 

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

@sunfish 
Hi Dan, I hope you are still happy with this change.  I didn't change any 
WebAssembly tests, but rather added a new IR-level test, so all existing 
WebAssembly behavior should stay the same.  Let me know if you have any 
concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

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

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

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



Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:900
+  // __cxa_atexit calls to avoid emitting the deprecated __mod_term_func.
+  if (TM->getTargetTriple().isOSBinFormatMachO())
+addPass(createLowerGlobalDtorsLegacyPass());

yln wrote:
> delcypher wrote:
> > Random thought. Do we want to support the legacy way of calling 
> > destructors, rather than removing it entirely? If we were to do such a 
> > thing I'd suspect we'd guard using the legacy way on the OS deployment 
> > target.
> > 
> > Just to be clear. I'm happy with the patch the way it is. I'm just 
> > wondering if we should consider allowing the legacy way as well. I can't 
> > see an obvious use case for it because the new way should work on older OSs 
> > too but maybe there's a use case I haven't thought about?
> Having a way to explicitly request the old behavior sounds like a good idea.  
> I will look into it.
I added an escape hatch to fallback to the old behavior:
* via Clang driver flag `-fregister-global-dtors-with-atexit`
* llc / code generation flag -lower-global-dtors-via-cxa-atexit.
This escape hatch will be removed in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

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

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

[Julian Lettner via Phabricator via cfe-commits]

yln updated this revision to Diff 415273.
yln added a comment.
Herald added subscribers: cfe-commits, ormris.
Herald added a project: clang.

Add support for an escape hatch to fallback to the old behavior: via Clang 
driver flag
(`-fregister-global-dtors-with-atexit`) or llc / code generation flag
(`-lower-global-dtors-via-cxa-atexit`).  This escape hatch will be
removed in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121327

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/docs/Passes.rst
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/LinkAllPasses.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h
  llvm/include/llvm/Transforms/Utils.h
  llvm/include/llvm/Transforms/Utils/LowerGlobalDtors.h
  llvm/lib/CodeGen/CodeGen.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Target/WebAssembly/CMakeLists.txt
  llvm/lib/Target/WebAssembly/WebAssembly.h
  llvm/lib/Target/WebAssembly/WebAssemblyLowerGlobalDtors.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/LowerGlobalDtors.cpp
  llvm/test/CodeGen/ARM/ctors_dtors.ll
  llvm/test/CodeGen/X86/2011-08-29-InitOrder.ll
  llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll

Index: llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll
@@ -0,0 +1,156 @@
+; RUN: opt-lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+; RUN: opt -passes=lower-global-dtors -S < %s | FileCheck %s --implicit-check-not=llvm.global_dtors
+
+; Test that @llvm.global_dtors is properly lowered into @llvm.global_ctors,
+; grouping dtor calls by priority and associated symbol.
+
+declare void @orig_ctor()
+declare void @orig_dtor0()
+declare void @orig_dtor1a()
+declare void @orig_dtor1b()
+declare void @orig_dtor1c0()
+declare void @orig_dtor1c1a()
+declare void @orig_dtor1c1b()
+declare void @orig_dtor1c2a()
+declare void @orig_dtor1c2b()
+declare void @orig_dtor1c3()
+declare void @orig_dtor1d()
+declare void @orig_dtor65535()
+declare void @orig_dtor65535c0()
+declare void @after_the_null()
+
+@associatedc0 = external global i8
+@associatedc1 = external global i8
+@associatedc2 = global i8 42
+@associatedc3 = global i8 84
+
+@llvm.global_ctors = appending global
+[1 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null }
+]
+
+@llvm.global_dtors = appending global
+[14 x { i32, void ()*, i8* }]
+[
+  { i32, void ()*, i8* } { i32 0, void ()* @orig_dtor0, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1a, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1b, i8* null },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1a, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c1b, i8* @associatedc1 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2a, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c2b, i8* @associatedc2 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1c3, i8* @associatedc3 },
+  { i32, void ()*, i8* } { i32 1, void ()* @orig_dtor1d, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535c0, i8* @associatedc0 },
+  { i32, void ()*, i8* } { i32 65535, void ()* @orig_dtor65535, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* null, i8* null },
+  { i32, void ()*, i8* } { i32 65535, void ()* @after_the_null, i8* null }
+]
+
+; CHECK: @associatedc0 = external global i8
+; CHECK: @associatedc1 = external global i8
+; CHECK: @associatedc2 = global i8 42
+; CHECK: @associatedc3 = global i8 84
+; CHECK: @__dso_handle = extern_weak hidden constant i8
+
+; CHECK-LABEL: @llvm.global_ctors = appending global [10 x { i32, void ()*, i8* }] [
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 200, void ()* @orig_ctor, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 0, void ()* @register_call_dtors.0, i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$0", i8* null },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$1.associatedc0", i8* @associatedc0 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* @"register_call_dtors.1$2.associatedc1", i8* @associatedc1 },
+; CHECK-SAME:  { i32, void ()*, i8* } { i32 1, void ()* 

[PATCH] D98995: [CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: clang/lib/CodeGen/CGAtomic.cpp:444-447
+// Prior to c++17, "the failure argument shall be no stronger than the
+// success argument". This condition has been lifted and the only
+// precondition is 31.7.2.18. Effectively treat this as a DR and skip
+// language version checks.

yln wrote:
> Should the following assert in `AtomicCmpXchgInst::Init()` have been changed 
> as well?
> ```
> assert(!isStrongerThan(FailureOrdering, SuccessOrdering) &&
>"AtomicCmpXchg failure argument shall be no stronger than the success "
>"argument");
> ```
> https://github.com/llvm/llvm-project/blob/dad5caa59e6b2bde8d6cf5b64a972c393c526c82/llvm/lib/IR/Instructions.cpp#L1561
> 
> I am observing a crash on an internal code base most likely caused by this.
//friendly ping// @bruno 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98995

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

[PATCH] D98995: [CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: clang/lib/CodeGen/CGAtomic.cpp:444-447
+// Prior to c++17, "the failure argument shall be no stronger than the
+// success argument". This condition has been lifted and the only
+// precondition is 31.7.2.18. Effectively treat this as a DR and skip
+// language version checks.

Should the following assert in `AtomicCmpXchgInst::Init()` have been changed as 
well?
```
assert(!isStrongerThan(FailureOrdering, SuccessOrdering) &&
   "AtomicCmpXchg failure argument shall be no stronger than the success "
   "argument");
```
https://github.com/llvm/llvm-project/blob/dad5caa59e6b2bde8d6cf5b64a972c393c526c82/llvm/lib/IR/Instructions.cpp#L1561

I am observing a crash on an internal code base most likely caused by this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98995

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

[PATCH] D97327: [NFC] Switch to auto marshalling infrastructure for `-fsanitize-address-destructor-kind=` flag.

[Julian Lettner via Phabricator via cfe-commits]

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

LGTM, cleanup only, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97327

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

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

My apologies for misrepresenting the meaning of the docs and stirring up this 
confusion.  I did not read the docs carefully enough.

Replying on the mailing list ...




Comment at: clang/CMakeLists.txt:1
 cmake_minimum_required(VERSION 3.13.4)
 

Note that we already have a precedent for requiring the stated version with 
CMake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95635

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

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

In D95635#2528851 , @JDevlieghere 
wrote:

>> However, the project claims to require 3.6 or greater, and 3.6 features are 
>> being used.
>
> Can you link to where it says that? I'm not opposed to making 3.6 the 
> minimally required version, but at least for LLDB we don't have that 
> requirement and we have documentation mentioning Python 3.5.

For LLVM: https://llvm.org/docs/GettingStarted.html#software


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95635

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

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

LGTM, thanks!  (I got bitten by this.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95635

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

[PATCH] D86980: [Docs] Remove outdated OS limitation

[Julian Lettner 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 rG478eb98cd25c: [Docs] Remove outdated OS limitation (authored 
by yln).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86980

Files:
  clang/docs/UsersManual.rst


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3227,11 +3227,6 @@
 Operating System Features and Limitations
 -
 
-Darwin (macOS)
-^^
-
-Thread Sanitizer is not supported.
-
 Windows
 ^^^
 


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3227,11 +3227,6 @@
 Operating System Features and Limitations
 -
 
-Darwin (macOS)
-^^
-
-Thread Sanitizer is not supported.
-
 Windows
 ^^^
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86980: [Docs] Remove outdated OS limitation

[Julian Lettner via Phabricator via cfe-commits]

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

Thread Sanitizer is supported on macOS.

rdar://68159753


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86980

Files:
  clang/docs/UsersManual.rst


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3227,11 +3227,6 @@
 Operating System Features and Limitations
 -
 
-Darwin (macOS)
-^^
-
-Thread Sanitizer is not supported.
-
 Windows
 ^^^
 


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3227,11 +3227,6 @@
 Operating System Features and Limitations
 -
 
-Darwin (macOS)
-^^
-
-Thread Sanitizer is not supported.
-
 Windows
 ^^^
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82791: [lit] Improve lit's output with default settings and --verbose.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: llvm/docs/CommandGuide/lit.rst:102
+
+ Alias for ``-v``/``--verbose`` (for backwards compatibility).
 .. option:: -a, --show-all

blank line



Comment at: llvm/utils/lit/lit/OutputSettings.py:34
+ONLY_FAILING_COMMAND = CommandOutputStyle("OnlyFailing")
+UP_TO_AND_INCLUDING_FAILING_COMMAND = CommandOutputStyle("UpToAndIncluding")
+

varungandhi-apple wrote:
> yln wrote:
> > I really like the "communicating intent" part of this infrastructure.  
> > However, this is a lot of code and `if`s considering we really only have to 
> > distinguish two cases.  From your commit message:
> > 
> > - default (no flags): no script, show only failing line in command output
> > - `-v`: full script, adds 'set +x', shows command output
> > 
> > Am I missing something or could everything be keyed off a single verbose 
> > (reading from the code below `showAllOutput` implies verbose) flag.  Do we 
> > anticipate other combinations being useful? (In general I would argue for 
> > striving for the simplest implementation that gives us what we currently 
> > want and not try to anticipate future extensions.)
> > 
> > Have you considered (or started out with) just using a single verbose flag 
> > to base your decisions in the implementation functions?  
> Not sure what you mean by two cases, I think there are three cases:
> 
> 1. Quiet -> Corresponds on `NO_COMMAND`
> 2. Default (no quiet, no verbose) -> Corresponds to `ONLY_FAILING`.
> 3. Verbose -> Corresponds to `UP_TO_AND_INCLUDING_FAILING`.
> 
> Since there is a 1-1 correspondence, we _could_ compare quiet vs (!quiet && 
> !verbose) vs verbose in `make_command_output`. I chose not to do that because 
> I figured this makes the intent clearer by distinguishing between 
> user-specified options and derived options.
> 
> Does that make sense? Would you still prefer that I get rid of this?
Okay, I agree now that having an enum-like thing is really nice because we can 
give it a good explanatory name.

Can we roll the following options into one enum-like `OutputStyle.XXX` 
corresponding to our 3 cases: "quiet, default/only_failing, 
verbose/up_to_first_failing"; since the user's can't configure them 
individually.

I imagine the following things
`opts.show_output_on_failure`, `opts.script_output_style`, 
`opts.command_output_style`, and `opts.quiet` being replaced with 
`opts.output_style`



Comment at: llvm/utils/lit/lit/TestRunner.py:1503-1508
+Returns a pair of:
+- The index in ``output_str`` pointing to immediately after the preceding
+  newline, i.e. the start of the RUN line, before any shell-specific
+  prefix.
+- The matched substring itself, including the number at the end,
+  starting with 'RUN', skipping the shell-specific prefix.

varungandhi-apple wrote:
> yln wrote:
> > Why use a complicated parser-like return value? Our only caller below could 
> > just receive the potentially found RUN line.
> Sorry, it's not super clear because in this revision, there is only one 
> caller, which could do with just using the index. However, the highlighting 
> patch (which comes right after this logically) ends up making use of the 
> substring. The line of reasoning is:
> 
> 1. A substring by itself is not good enough (and hence `make_command_output` 
> in this patch makes use of the first index return value), because we want to 
> distinguish the case `line_start == 0` from the case `line_start > 0` by 
> printing `Command Output (stderr)` instead of `Command Output (stderr, 
> truncated)` when `line_start == 0`.
> 2. An index by itself could be "good enough", but we try to be smarter in the 
> highlighting patch (https://reviews.llvm.org/D82811) by not highlighting the 
> shell-specific prefix, and starting highlighting from the "RUN", which 
> `make_script_output` makes use of.
> 
> Does that make sense? I decided against splitting changes to this function 
> into two revisions and have the full functionality in the first iteration 
> because I felt like that would create more churn with the tests and the 
> implementation, but I see how it can be confusing as to why two 
> related-but-slightly-different-values are being returned simultaneously when 
> only one is really being used in this patch.
Yes, please implement it in the "simplest way possible" for the current 
functionality without anticipating future requirements.  We can always make it 
more complex if required for a future feature, but's it's harder to "see" that 
there is opportunity for simplification once the code is there.

Also, another reviewer might suggest a solution you haven't thought about.

For example, could highlighting be implemented via (without worrying about 
indexes)?
```
s = get_important_bits(full_output)
s_highlighted = '{}'.format(s)
highlighted_output = full_output.replace(s, s_highlighted)
```





Comment at: 

[PATCH] D84082: [tsan] Allow TSan in the Clang driver for Apple Silicon Macs

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2734
 
-  if (isTargetMacOS()) {
-if (IsX86_64)
-  Res |= SanitizerKind::Thread;
+  if ((IsX86_64 || IsAArch64) && isTargetMacOS()) {
+Res |= SanitizerKind::Thread;

This breaks the symmetry with the if below.  Is this intentional?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84082



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

[PATCH] D84082: [tsan] Allow TSan in the Clang driver for Apple Silicon Macs

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Simulators on Apple Silicon are not yet supported?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84082



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

[PATCH] D82791: [lit] Improve lit's output with default settings and --verbose.

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

The changes here are for text printed for failing tests, right?
And `showAllOutput` is orthogonal and just shows everything even for 
non-failing tests?




Comment at: llvm/utils/lit/lit/OutputSettings.py:34
+ONLY_FAILING_COMMAND = CommandOutputStyle("OnlyFailing")
+UP_TO_AND_INCLUDING_FAILING_COMMAND = CommandOutputStyle("UpToAndIncluding")
+

I really like the "communicating intent" part of this infrastructure.  However, 
this is a lot of code and `if`s considering we really only have to distinguish 
two cases.  From your commit message:

- default (no flags): no script, show only failing line in command output
- `-v`: full script, adds 'set +x', shows command output

Am I missing something or could everything be keyed off a single verbose 
(reading from the code below `showAllOutput` implies verbose) flag.  Do we 
anticipate other combinations being useful? (In general I would argue for 
striving for the simplest implementation that gives us what we currently want 
and not try to anticipate future extensions.)

Have you considered (or started out with) just using a single verbose flag to 
base your decisions in the implementation functions?  



Comment at: llvm/utils/lit/lit/TestRunner.py:1499
 
+def locate_last_run_line(output_str):
+"""

I feel like this function is the most complex part of this patch.  I don't 
fully follow it, but you experimented and believe this is the best approach and 
wrote a whole test suite so I am happy with it.



Comment at: llvm/utils/lit/lit/TestRunner.py:1503-1508
+Returns a pair of:
+- The index in ``output_str`` pointing to immediately after the preceding
+  newline, i.e. the start of the RUN line, before any shell-specific
+  prefix.
+- The matched substring itself, including the number at the end,
+  starting with 'RUN', skipping the shell-specific prefix.

Why use a complicated parser-like return value? Our only caller below could 
just receive the potentially found RUN line.



Comment at: llvm/utils/lit/lit/TestRunner.py:1593
+assert(lit_config.script_output_style == OutputSettings.FULL_SCRIPT)
+return default_output()
+

Why are we using two local functions here?

The whole thing could be (already assuming just one verbose flag):

```
def make_script_output(lit_config, script_lines, exit_code):
if not lit_config.verbose:
return ""
return ...
```



Comment at: llvm/utils/lit/lit/TestRunner.py:1614-1617
+line_start, run_line_str = locate_last_run_line(cmd_output)
+# maybe there was an error, or maybe we are not truncating anything
+if run_line_str is None or line_start == 0:
+return default_output()

This block should be pushed into the `lit_config.command_output_style == 
OutputSettings.ONLY_FAILING_COMMAND` case, otherwise we are always searching 
for the last line, even if we don't really use the result of the computation.

Also: if we fail to find the RUN line, we might print the note to use 
`--verbose` even if the user already specified it.



Comment at: llvm/utils/lit/lit/TestRunner.py:1624
+   == OutputSettings.UP_TO_AND_INCLUDING_FAILING_COMMAND)
+return make_output(cmd_output, is_truncated=False)
 

Please try to simplify this a bit, maybe the following?

```
def make_command_output():
if not lit_config.quiet:
return ""

verbose = lit_config.verbose
output = "header {} ...".format(", truncated" if verbose else "")
if verbose:
output += cmd_output
else:
run_line = locate_last_run_line() # then deal with "not found" case
output += ...
output += "Note:  try to use --verbose"

 return output
```



Comment at: llvm/utils/lit/lit/cl_arguments.py:56
 action="store_true")
+# TODO(python3): Use aliases kwarg for add_argument above.
 format_group.add_argument("-vv", "--echo-all-commands",

I think aliases kwarg is something else (seems to be related to subparsers).  
You could just add `"-vv", "--echo-all-commands"` after `"--verbose"` to allow 
for additional names for the flag, but I think I prefer to have it separate 
(makes it easer to remove it if we ever decide to drop the alias in the future).

So long comment short: just drop this comment please.



Comment at: llvm/utils/lit/lit/cl_arguments.py:178
+opts.command_output_style = OutputSettings.ONLY_FAILING_COMMAND
+opts.echo_all_commands = True
+

Unconditionally overwritten below



Comment at: llvm/utils/lit/lit/cl_arguments.py:195
+cmd_output_style == OutputSettings.UP_TO_AND_INCLUDING_FAILING_COMMAND
+or cmd_output_style == OutputSettings.ONLY_FAILING_COMMAND)
 

`opts.echo_all_commands = 

[PATCH] D77708: [lit] Improve naming of test result categories

[Julian Lettner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG99d6e05e7144: [lit] Improve naming of test result categories 
(authored by yln).
Herald added subscribers: cfe-commits, msifontes, jurahul, Kayjukh, frgossen, 
grosul1, Joonsoo, stephenneuendorffer, liufengdb, lucyrfox, mgester, 
arpith-jacob, nicolasvasilache, antiagainst, shauheen, jpienaar, rriddle, 
mehdi_amini.
Herald added projects: clang, MLIR.

Changed prior to commit:
  https://reviews.llvm.org/D77708?vs=264307=268809#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77708

Files:
  clang/www/hacking.html
  llvm/utils/lit/lit/main.py
  llvm/utils/lit/tests/allow-retries.py
  llvm/utils/lit/tests/custom-result-category.py
  llvm/utils/lit/tests/googletest-discovery-failed.py
  llvm/utils/lit/tests/googletest-format.py
  llvm/utils/lit/tests/googletest-timeout.py
  llvm/utils/lit/tests/googletest-upstream-format.py
  llvm/utils/lit/tests/lit-opts.py
  llvm/utils/lit/tests/max-failures.py
  llvm/utils/lit/tests/max-time.py
  llvm/utils/lit/tests/parallelism-groups.py
  llvm/utils/lit/tests/selecting.py
  llvm/utils/lit/tests/shtest-env.py
  llvm/utils/lit/tests/shtest-format.py
  llvm/utils/lit/tests/shtest-inject.py
  llvm/utils/lit/tests/shtest-not.py
  llvm/utils/lit/tests/shtest-shell.py
  llvm/utils/lit/tests/shtest-timeout.py
  mlir/test/Examples/standalone/test.toy

Index: mlir/test/Examples/standalone/test.toy
===
--- mlir/test/Examples/standalone/test.toy
+++ mlir/test/Examples/standalone/test.toy
@@ -1,4 +1,4 @@
 # RUN: %cmake %mlir_src_root/examples/standalone -DCMAKE_CXX_COMPILER=%host_cxx -DCMAKE_C_COMPILER=%host_cc -DMLIR_DIR=%llvm_lib_dir/cmake/mlir ; %cmake --build . --target check-standalone | tee %t | FileCheck %s
 
-# CHECK: Expected Passes: 3
+# CHECK: Passed: 3
 # UNSUPPORTED: windows, android
Index: llvm/utils/lit/tests/shtest-timeout.py
===
--- llvm/utils/lit/tests/shtest-timeout.py
+++ llvm/utils/lit/tests/shtest-timeout.py
@@ -50,8 +50,8 @@
 
 # CHECK-OUT-COMMON: PASS: per_test_timeout :: short.py
 
-# CHECK-OUT-COMMON: Expected Passes{{ *}}: 1
-# CHECK-OUT-COMMON: Individual Timeouts{{ *}}: 1
+# CHECK-OUT-COMMON: Passed   : 1
+# CHECK-OUT-COMMON: Timed Out: 1
 
 # Test per test timeout via a config file and on the command line.
 # The value set on the command line should override the config file.
@@ -71,5 +71,5 @@
 
 # CHECK-CMDLINE-OVERRIDE-OUT: PASS: per_test_timeout :: short.py
 
-# CHECK-CMDLINE-OVERRIDE-OUT: Expected Passes{{ *}}: 1
-# CHECK-CMDLINE-OVERRIDE-OUT: Individual Timeouts{{ *}}: 1
+# CHECK-CMDLINE-OVERRIDE-OUT: Passed   : 1
+# CHECK-CMDLINE-OVERRIDE-OUT: Timed Out: 1
Index: llvm/utils/lit/tests/shtest-shell.py
===
--- llvm/utils/lit/tests/shtest-shell.py
+++ llvm/utils/lit/tests/shtest-shell.py
@@ -583,4 +583,4 @@
 # CHECK: ***
 
 # CHECK: PASS: shtest-shell :: valid-shell.txt
-# CHECK: Failing Tests (35)
+# CHECK: Failed Tests (35)
Index: llvm/utils/lit/tests/shtest-not.py
===
--- llvm/utils/lit/tests/shtest-not.py
+++ llvm/utils/lit/tests/shtest-not.py
@@ -110,6 +110,6 @@
 # CHECK: Error: 'not --crash' cannot call 'rm'
 # CHECK: error: command failed with exit status: {{.*}}
 
-# CHECK: Expected Passes : 1
-# CHECK: Unexpected Failures: 12
+# CHECK: Passed:  1
+# CHECK: Failed: 12
 # CHECK-NOT: {{.}}
Index: llvm/utils/lit/tests/shtest-inject.py
===
--- llvm/utils/lit/tests/shtest-inject.py
+++ llvm/utils/lit/tests/shtest-inject.py
@@ -11,7 +11,7 @@
 # CHECK-TEST1: THIS WAS
 # CHECK-TEST1: INJECTED
 #
-# CHECK-TEST1: Expected Passes: 1
+# CHECK-TEST1: Passed: 1
 
 # RUN: %{lit} -j 1 %{inputs}/shtest-inject/test-one.txt --show-all | FileCheck --check-prefix=CHECK-TEST2 %s
 #
@@ -26,7 +26,7 @@
 # CHECK-TEST2: INJECTED
 # CHECK-TEST2: IN THE FILE
 #
-# CHECK-TEST2: Expected Passes: 1
+# CHECK-TEST2: Passed: 1
 
 # RUN: %{lit} -j 1 %{inputs}/shtest-inject/test-many.txt --show-all | FileCheck --check-prefix=CHECK-TEST3 %s
 #
@@ -45,4 +45,4 @@
 # CHECK-TEST3: IF IT WORKS
 # CHECK-TEST3: AS EXPECTED
 #
-# CHECK-TEST3: Expected Passes: 1
+# CHECK-TEST3: Passed: 1
Index: llvm/utils/lit/tests/shtest-format.py
===
--- llvm/utils/lit/tests/shtest-format.py
+++ llvm/utils/lit/tests/shtest-format.py
@@ -69,21 +69,21 @@
 # CHECK-NEXT: true
 # CHECK-NEXT: --
 
-# CHECK: Failing Tests (3)
+# CHECK: Failed Tests (3)
 # CHECK: shtest-format :: external_shell/fail.txt
 # CHECK: shtest-format :: external_shell/fail_with_bad_encoding.txt
 # CHECK: shtest-format :: fail.txt
 
-# CHECK: Unexpected Passing Tests (1)
+# CHECK: 

[PATCH] D57722: [Clang][NCF] Sanitizer options: helpers to check if {Kernel, Hardware}ASan is enabled

[Julian Lettner via Phabricator via cfe-commits]

yln created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Kernel ASan is basically ASan with a few specific settings, but has a
seperate front-end flag. Provide a helper that checks for both, so new
comitters like myself are less likely to forget the Kernel variant.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57722

Files:
  clang/include/clang/Basic/Features.def
  clang/include/clang/Basic/Sanitizers.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/SanitizerMetadata.cpp

Index: clang/lib/CodeGen/SanitizerMetadata.cpp
===
--- clang/lib/CodeGen/SanitizerMetadata.cpp
+++ clang/lib/CodeGen/SanitizerMetadata.cpp
@@ -24,10 +24,7 @@
SourceLocation Loc, StringRef Name,
QualType Ty, bool IsDynInit,
bool IsBlacklisted) {
-  if (!CGM.getLangOpts().Sanitize.hasOneOf(SanitizerKind::Address |
-   SanitizerKind::KernelAddress |
-   SanitizerKind::HWAddress |
-   SanitizerKind::KernelHWAddress))
+  if (!CGM.getLangOpts().Sanitize.hasASanOrKASanOrHWASanOrHWKASan())
 return;
   IsDynInit &= !CGM.isInSanitizerBlacklist(GV, Loc, Ty, "init");
   IsBlacklisted |= CGM.isInSanitizerBlacklist(GV, Loc, Ty);
@@ -58,10 +55,7 @@
 
 void SanitizerMetadata::reportGlobalToASan(llvm::GlobalVariable *GV,
const VarDecl , bool IsDynInit) {
-  if (!CGM.getLangOpts().Sanitize.hasOneOf(SanitizerKind::Address |
-   SanitizerKind::KernelAddress |
-   SanitizerKind::HWAddress |
-   SanitizerKind::KernelHWAddress))
+  if (!CGM.getLangOpts().Sanitize.hasASanOrKASanOrHWASanOrHWKASan())
 return;
   std::string QualName;
   llvm::raw_string_ostream OS(QualName);
@@ -78,10 +72,7 @@
 void SanitizerMetadata::disableSanitizerForGlobal(llvm::GlobalVariable *GV) {
   // For now, just make sure the global is not modified by the ASan
   // instrumentation.
-  if (CGM.getLangOpts().Sanitize.hasOneOf(SanitizerKind::Address |
-  SanitizerKind::KernelAddress |
-  SanitizerKind::HWAddress |
-  SanitizerKind::KernelHWAddress))
+  if (CGM.getLangOpts().Sanitize.hasASanOrKASanOrHWASanOrHWKASan())
 reportGlobalToASan(GV, SourceLocation(), "", QualType(), false, true);
 }
 
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -876,9 +876,9 @@
   }
 
   // Apply sanitizer attributes to the function.
-  if (SanOpts.hasOneOf(SanitizerKind::Address | SanitizerKind::KernelAddress))
+  if (SanOpts.hasASanOrKASan())
 Fn->addFnAttr(llvm::Attribute::SanitizeAddress);
-  if (SanOpts.hasOneOf(SanitizerKind::HWAddress | SanitizerKind::KernelHWAddress))
+  if (SanOpts.hasHWASanOrHWKASan())
 Fn->addFnAttr(llvm::Attribute::SanitizeHWAddress);
   if (SanOpts.has(SanitizerKind::Thread))
 Fn->addFnAttr(llvm::Attribute::SanitizeThread);
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -4403,8 +4403,7 @@
 
   // Avoid incompatibility with ASan which relies on the `noreturn`
   // attribute to insert handler calls.
-  if (SanOpts.hasOneOf(SanitizerKind::Address |
-   SanitizerKind::KernelAddress)) {
+  if (SanOpts.hasASanOrKASan()) {
 SanitizerScope SanScope(this);
 llvm::IRBuilder<>::InsertPointGuard IPGuard(Builder);
 Builder.SetInsertPoint(CI);
Index: clang/include/clang/Basic/Sanitizers.h
===
--- clang/include/clang/Basic/Sanitizers.h
+++ clang/include/clang/Basic/Sanitizers.h
@@ -70,6 +70,25 @@
 
   /// Bitmask of enabled sanitizers.
   SanitizerMask Mask = 0;
+
+  /// Check if ASan or Kernel ASan are enabled.
+  bool hasASanOrKASan() const {
+return hasOneOf(SanitizerKind::Address | SanitizerKind::KernelAddress);
+  }
+
+  /// Check if Hardware ASan or Hardware Kernel ASan are enabled.
+  bool hasHWASanOrHWKASan() const {
+return hasOneOf(SanitizerKind::HWAddress | SanitizerKind::KernelHWAddress);
+  }
+
+  /// Check if any of the following are enabled:
+  /// * ASan
+  /// * Kernel ASan
+  /// * Hardware ASan
+  /// * Hardware Kernel ASan
+  bool hasASanOrKASanOrHWASanOrHWKASan() const {
+return hasASanOrKASan() || hasHWASanOrHWKASan();
+  }
 };
 
 /// Parse a 

[PATCH] D57711: [Sanitizers] UBSan unreachable incompatible with Kernel ASan

[Julian Lettner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC353120: [Sanitizers] UBSan unreachable incompatible with 
Kernel ASan (authored by yln, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57711?vs=185153=185178#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57711

Files:
  lib/CodeGen/CGCall.cpp
  test/CodeGen/ubsan-asan-noreturn.c


Index: test/CodeGen/ubsan-asan-noreturn.c
===
--- test/CodeGen/ubsan-asan-noreturn.c
+++ test/CodeGen/ubsan-asan-noreturn.c
@@ -1,6 +1,7 @@
 // Ensure compatiblity of UBSan unreachable with ASan in the presence of
 // noreturn functions.
-// RUN: %clang_cc1 -fsanitize=unreachable,address -triple x86_64-linux 
-emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=unreachable,address-triple x86_64-linux 
-emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=unreachable,kernel-address -triple x86_64-linux 
-emit-llvm -o - %s | FileCheck %s
 
 void my_longjmp(void) __attribute__((noreturn));
 
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4403,7 +4403,8 @@
 
   // Avoid incompatibility with ASan which relies on the `noreturn`
   // attribute to insert handler calls.
-  if (SanOpts.has(SanitizerKind::Address)) {
+  if (SanOpts.hasOneOf(SanitizerKind::Address |
+   SanitizerKind::KernelAddress)) {
 SanitizerScope SanScope(this);
 llvm::IRBuilder<>::InsertPointGuard IPGuard(Builder);
 Builder.SetInsertPoint(CI);


Index: test/CodeGen/ubsan-asan-noreturn.c
===
--- test/CodeGen/ubsan-asan-noreturn.c
+++ test/CodeGen/ubsan-asan-noreturn.c
@@ -1,6 +1,7 @@
 // Ensure compatiblity of UBSan unreachable with ASan in the presence of
 // noreturn functions.
-// RUN: %clang_cc1 -fsanitize=unreachable,address -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=unreachable,address-triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=unreachable,kernel-address -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
 
 void my_longjmp(void) __attribute__((noreturn));
 
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4403,7 +4403,8 @@
 
   // Avoid incompatibility with ASan which relies on the `noreturn`
   // attribute to insert handler calls.
-  if (SanOpts.has(SanitizerKind::Address)) {
+  if (SanOpts.hasOneOf(SanitizerKind::Address |
+   SanitizerKind::KernelAddress)) {
 SanitizerScope SanScope(this);
 llvm::IRBuilder<>::InsertPointGuard IPGuard(Builder);
 Builder.SetInsertPoint(CI);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57711: [Sanitizers] UBSan unreachable incompatible with Kernel ASan

[Julian Lettner via Phabricator via cfe-commits]

yln created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is a follow up for https://reviews.llvm.org/D57278. The previous
revision should have also included Kernel ASan.

rdar://problem/40723397


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57711

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/ubsan-asan-noreturn.c
  llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll


Index: llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll
===
--- llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll
+++ llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll
@@ -35,6 +35,15 @@
 ; CHECK-NOT:call void @__asan_handle_no_return
 ; CHECK:call void @NoReturnFunc
 
+; Do *not* instrument functions without ASan
+define i32 @Call4() {
+  call void @NoReturnFunc() noreturn
+  unreachable
+}
+; CHECK-LABEL:  @Call4
+; CHECK-NOT:call void @__asan_handle_no_return
+; CHECK:call void @NoReturnFunc
+
 declare i32 @__gxx_personality_v0(...)
 
 define i64 @Invoke1() nounwind uwtable ssp sanitize_address personality i8* 
bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
Index: clang/test/CodeGen/ubsan-asan-noreturn.c
===
--- clang/test/CodeGen/ubsan-asan-noreturn.c
+++ clang/test/CodeGen/ubsan-asan-noreturn.c
@@ -1,6 +1,7 @@
 // Ensure compatiblity of UBSan unreachable with ASan in the presence of
 // noreturn functions.
-// RUN: %clang_cc1 -fsanitize=unreachable,address -triple x86_64-linux 
-emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=unreachable,address-triple x86_64-linux 
-emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=unreachable,kernel-address -triple x86_64-linux 
-emit-llvm -o - %s | FileCheck %s
 
 void my_longjmp(void) __attribute__((noreturn));
 
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -4403,7 +4403,8 @@
 
   // Avoid incompatibility with ASan which relies on the `noreturn`
   // attribute to insert handler calls.
-  if (SanOpts.has(SanitizerKind::Address)) {
+  if (SanOpts.hasOneOf(SanitizerKind::Address |
+   SanitizerKind::KernelAddress)) {
 SanitizerScope SanScope(this);
 llvm::IRBuilder<>::InsertPointGuard IPGuard(Builder);
 Builder.SetInsertPoint(CI);


Index: llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll
===
--- llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll
+++ llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll
@@ -35,6 +35,15 @@
 ; CHECK-NOT:call void @__asan_handle_no_return
 ; CHECK:call void @NoReturnFunc
 
+; Do *not* instrument functions without ASan
+define i32 @Call4() {
+  call void @NoReturnFunc() noreturn
+  unreachable
+}
+; CHECK-LABEL:  @Call4
+; CHECK-NOT:call void @__asan_handle_no_return
+; CHECK:call void @NoReturnFunc
+
 declare i32 @__gxx_personality_v0(...)
 
 define i64 @Invoke1() nounwind uwtable ssp sanitize_address personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
Index: clang/test/CodeGen/ubsan-asan-noreturn.c
===
--- clang/test/CodeGen/ubsan-asan-noreturn.c
+++ clang/test/CodeGen/ubsan-asan-noreturn.c
@@ -1,6 +1,7 @@
 // Ensure compatiblity of UBSan unreachable with ASan in the presence of
 // noreturn functions.
-// RUN: %clang_cc1 -fsanitize=unreachable,address -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=unreachable,address-triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=unreachable,kernel-address -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
 
 void my_longjmp(void) __attribute__((noreturn));
 
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -4403,7 +4403,8 @@
 
   // Avoid incompatibility with ASan which relies on the `noreturn`
   // attribute to insert handler calls.
-  if (SanOpts.has(SanitizerKind::Address)) {
+  if (SanOpts.hasOneOf(SanitizerKind::Address |
+   SanitizerKind::KernelAddress)) {
 SanitizerScope SanScope(this);
 llvm::IRBuilder<>::InsertPointGuard IPGuard(Builder);
 Builder.SetInsertPoint(CI);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57489: [ASan] Do not instrument other runtime functions with `__asan_handle_no_return`

[Julian Lettner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352948: [ASan] Do not instrument other runtime functions 
with `__asan_handle_no_return` (authored by yln, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57489?vs=184648=184878#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57489

Files:
  lib/CodeGen/CGCall.cpp
  test/CodeGen/ubsan-asan-noreturn.c


Index: test/CodeGen/ubsan-asan-noreturn.c
===
--- test/CodeGen/ubsan-asan-noreturn.c
+++ test/CodeGen/ubsan-asan-noreturn.c
@@ -9,8 +9,7 @@
   my_longjmp();
   // CHECK:  @__asan_handle_no_return{{.*}} !nosanitize
   // CHECK-NEXT: @my_longjmp(){{[^#]*}}
-  // CHECK:  @__asan_handle_no_return()
-  // CHECK-NEXT: @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
+  // CHECK:  @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
   // CHECK-NEXT: unreachable
 }
 
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4394,8 +4394,8 @@
 
 // Strip away the noreturn attribute to better diagnose unreachable UB.
 if (SanOpts.has(SanitizerKind::Unreachable)) {
-  // Also remove from function since CI->hasFnAttr(..) also checks 
attributes
-  // of the called function.
+  // Also remove from function since CallBase::hasFnAttr additionally 
checks
+  // attributes of the called function.
   if (auto *F = CI->getCalledFunction())
 F->removeFnAttr(llvm::Attribute::NoReturn);
   CI->removeAttribute(llvm::AttributeList::FunctionIndex,


Index: test/CodeGen/ubsan-asan-noreturn.c
===
--- test/CodeGen/ubsan-asan-noreturn.c
+++ test/CodeGen/ubsan-asan-noreturn.c
@@ -9,8 +9,7 @@
   my_longjmp();
   // CHECK:  @__asan_handle_no_return{{.*}} !nosanitize
   // CHECK-NEXT: @my_longjmp(){{[^#]*}}
-  // CHECK:  @__asan_handle_no_return()
-  // CHECK-NEXT: @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
+  // CHECK:  @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
   // CHECK-NEXT: unreachable
 }
 
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4394,8 +4394,8 @@
 
 // Strip away the noreturn attribute to better diagnose unreachable UB.
 if (SanOpts.has(SanitizerKind::Unreachable)) {
-  // Also remove from function since CI->hasFnAttr(..) also checks attributes
-  // of the called function.
+  // Also remove from function since CallBase::hasFnAttr additionally checks
+  // attributes of the called function.
   if (auto *F = CI->getCalledFunction())
 F->removeFnAttr(llvm::Attribute::NoReturn);
   CI->removeAttribute(llvm::AttributeList::FunctionIndex,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57278: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352690: [Sanitizers] UBSan unreachable incompatible with 
ASan in the presence of… (authored by yln, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57278?vs=184168=184393#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57278

Files:
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CodeGenFunction.h
  test/CodeGen/ubsan-asan-noreturn.c
  test/CodeGenCXX/ubsan-unreachable.cpp

Index: test/CodeGen/ubsan-asan-noreturn.c
===
--- test/CodeGen/ubsan-asan-noreturn.c
+++ test/CodeGen/ubsan-asan-noreturn.c
@@ -0,0 +1,21 @@
+// Ensure compatiblity of UBSan unreachable with ASan in the presence of
+// noreturn functions.
+// RUN: %clang_cc1 -fsanitize=unreachable,address -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+
+void my_longjmp(void) __attribute__((noreturn));
+
+// CHECK-LABEL: define void @calls_noreturn()
+void calls_noreturn() {
+  my_longjmp();
+  // CHECK:  @__asan_handle_no_return{{.*}} !nosanitize
+  // CHECK-NEXT: @my_longjmp(){{[^#]*}}
+  // CHECK:  @__asan_handle_no_return()
+  // CHECK-NEXT: @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
+  // CHECK-NEXT: unreachable
+}
+
+// CHECK: declare void @my_longjmp() [[FN_ATTR:#[0-9]+]]
+// CHECK: declare void @__asan_handle_no_return()
+
+// CHECK-LABEL: attributes
+// CHECK-NOT: [[FN_ATTR]] = { {{.*noreturn.*}} }
Index: test/CodeGenCXX/ubsan-unreachable.cpp
===
--- test/CodeGenCXX/ubsan-unreachable.cpp
+++ test/CodeGenCXX/ubsan-unreachable.cpp
@@ -1,39 +1,37 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=unreachable | FileCheck %s
 
-extern void __attribute__((noreturn)) abort();
+void abort() __attribute__((noreturn));
 
-// CHECK-LABEL: define void @_Z14calls_noreturnv
+// CHECK-LABEL: define void @_Z14calls_noreturnv()
 void calls_noreturn() {
+  // Check absence ([^#]*) of call site attributes (including noreturn)
+  // CHECK: call void @_Z5abortv(){{[^#]*}}
   abort();
 
-  // Check that there are no attributes on the call site.
-  // CHECK-NOT: call void @_Z5abortv{{.*}}#
-
   // CHECK: __ubsan_handle_builtin_unreachable
   // CHECK: unreachable
 }
 
 struct A {
-  // CHECK: declare void @_Z5abortv{{.*}} [[ABORT_ATTR:#[0-9]+]]
+  // CHECK: declare void @_Z5abortv() [[EXTERN_FN_ATTR:#[0-9]+]]
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call1Ev
   void call1() {
-// CHECK-NOT: call void @_ZN1A16does_not_return2Ev{{.*}}#
+// CHECK: call void @_ZN1A16does_not_return2Ev({{.*}}){{[^#]*}}
 does_not_return2();
 
 // CHECK: __ubsan_handle_builtin_unreachable
 // CHECK: unreachable
   }
 
-  // Test static members.
-  static void __attribute__((noreturn)) does_not_return1() {
-// CHECK-NOT: call void @_Z5abortv{{.*}}#
+  // Test static members. Checks are below after `struct A` scope ends.
+  static void does_not_return1() __attribute__((noreturn)) {
 abort();
   }
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call2Ev
   void call2() {
-// CHECK-NOT: call void @_ZN1A16does_not_return1Ev{{.*}}#
+// CHECK: call void @_ZN1A16does_not_return1Ev(){{[^#]*}}
 does_not_return1();
 
 // CHECK: __ubsan_handle_builtin_unreachable
@@ -41,23 +39,23 @@
   }
 
   // Test calls through pointers to non-static member functions.
-  typedef void __attribute__((noreturn)) (A::*MemFn)();
+  typedef void (A::*MemFn)() __attribute__((noreturn));
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call3Ev
   void call3() {
 MemFn MF = ::does_not_return2;
+// CHECK: call void %{{[0-9]+\(.*}}){{[^#]*}}
 (this->*MF)();
 
-// CHECK-NOT: call void %{{.*}}#
 // CHECK: __ubsan_handle_builtin_unreachable
 // CHECK: unreachable
   }
 
   // Test regular members.
   // CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return2Ev({{.*}})
-  // CHECK-SAME: [[DOES_NOT_RETURN_ATTR:#[0-9]+]]
-  void __attribute__((noreturn)) does_not_return2() {
-// CHECK-NOT: call void @_Z5abortv(){{.*}}#
+  // CHECK-SAME: [[USER_FN_ATTR:#[0-9]+]]
+  void does_not_return2() __attribute__((noreturn)) {
+// CHECK: call void @_Z5abortv(){{[^#]*}}
 abort();
 
 // CHECK: call void @__ubsan_handle_builtin_unreachable
@@ -68,7 +66,9 @@
   }
 };
 
-// CHECK: define linkonce_odr void @_ZN1A16does_not_return1Ev() [[DOES_NOT_RETURN_ATTR]]
+// CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return1Ev()
+// CHECK-SAME: [[USER_FN_ATTR]]
+// CHECK: call void @_Z5abortv(){{[^#]*}}
 
 void force_irgen() {
   A a;
@@ -77,5 +77,7 @@
   a.call3();
 }
 
-// CHECK-NOT: [[ABORT_ATTR]] = {{[^}]+}}noreturn
-// CHECK-NOT: [[DOES_NOT_RETURN_ATTR]] = {{[^}]+}}noreturn
+// `noreturn` should be removed from functions and call sites
+// CHECK-LABEL: attributes
+// 

[PATCH] D57278: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

@eugenis @delcypher
Looks good to you?


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

https://reviews.llvm.org/D57278



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

[PATCH] D57278: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln updated this revision to Diff 184168.
yln added a comment.

Directly insert calls to `__asan_handle_no_return`

Clang-CodeGen now directly insert calls to `__asan_handle_no_return`
when a call to a noreturn function is encountered and both UBsan
unreachable and ASan are enabled. This allows UBSan to continue
removing the noreturn attribute from functions without any changes to
the ASan pass.


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

https://reviews.llvm.org/D57278

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/ubsan-asan-noreturn.c
  clang/test/CodeGenCXX/ubsan-unreachable.cpp

Index: clang/test/CodeGenCXX/ubsan-unreachable.cpp
===
--- clang/test/CodeGenCXX/ubsan-unreachable.cpp
+++ clang/test/CodeGenCXX/ubsan-unreachable.cpp
@@ -1,39 +1,37 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=unreachable | FileCheck %s
 
-extern void __attribute__((noreturn)) abort();
+void abort() __attribute__((noreturn));
 
-// CHECK-LABEL: define void @_Z14calls_noreturnv
+// CHECK-LABEL: define void @_Z14calls_noreturnv()
 void calls_noreturn() {
+  // Check absence ([^#]*) of call site attributes (including noreturn)
+  // CHECK: call void @_Z5abortv(){{[^#]*}}
   abort();
 
-  // Check that there are no attributes on the call site.
-  // CHECK-NOT: call void @_Z5abortv{{.*}}#
-
   // CHECK: __ubsan_handle_builtin_unreachable
   // CHECK: unreachable
 }
 
 struct A {
-  // CHECK: declare void @_Z5abortv{{.*}} [[ABORT_ATTR:#[0-9]+]]
+  // CHECK: declare void @_Z5abortv() [[EXTERN_FN_ATTR:#[0-9]+]]
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call1Ev
   void call1() {
-// CHECK-NOT: call void @_ZN1A16does_not_return2Ev{{.*}}#
+// CHECK: call void @_ZN1A16does_not_return2Ev({{.*}}){{[^#]*}}
 does_not_return2();
 
 // CHECK: __ubsan_handle_builtin_unreachable
 // CHECK: unreachable
   }
 
-  // Test static members.
-  static void __attribute__((noreturn)) does_not_return1() {
-// CHECK-NOT: call void @_Z5abortv{{.*}}#
+  // Test static members. Checks are below after `struct A` scope ends.
+  static void does_not_return1() __attribute__((noreturn)) {
 abort();
   }
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call2Ev
   void call2() {
-// CHECK-NOT: call void @_ZN1A16does_not_return1Ev{{.*}}#
+// CHECK: call void @_ZN1A16does_not_return1Ev(){{[^#]*}}
 does_not_return1();
 
 // CHECK: __ubsan_handle_builtin_unreachable
@@ -41,23 +39,23 @@
   }
 
   // Test calls through pointers to non-static member functions.
-  typedef void __attribute__((noreturn)) (A::*MemFn)();
+  typedef void (A::*MemFn)() __attribute__((noreturn));
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call3Ev
   void call3() {
 MemFn MF = ::does_not_return2;
+// CHECK: call void %{{[0-9]+\(.*}}){{[^#]*}}
 (this->*MF)();
 
-// CHECK-NOT: call void %{{.*}}#
 // CHECK: __ubsan_handle_builtin_unreachable
 // CHECK: unreachable
   }
 
   // Test regular members.
   // CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return2Ev({{.*}})
-  // CHECK-SAME: [[DOES_NOT_RETURN_ATTR:#[0-9]+]]
-  void __attribute__((noreturn)) does_not_return2() {
-// CHECK-NOT: call void @_Z5abortv(){{.*}}#
+  // CHECK-SAME: [[USER_FN_ATTR:#[0-9]+]]
+  void does_not_return2() __attribute__((noreturn)) {
+// CHECK: call void @_Z5abortv(){{[^#]*}}
 abort();
 
 // CHECK: call void @__ubsan_handle_builtin_unreachable
@@ -68,7 +66,9 @@
   }
 };
 
-// CHECK: define linkonce_odr void @_ZN1A16does_not_return1Ev() [[DOES_NOT_RETURN_ATTR]]
+// CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return1Ev()
+// CHECK-SAME: [[USER_FN_ATTR]]
+// CHECK: call void @_Z5abortv(){{[^#]*}}
 
 void force_irgen() {
   A a;
@@ -77,5 +77,7 @@
   a.call3();
 }
 
-// CHECK-NOT: [[ABORT_ATTR]] = {{[^}]+}}noreturn
-// CHECK-NOT: [[DOES_NOT_RETURN_ATTR]] = {{[^}]+}}noreturn
+// `noreturn` should be removed from functions and call sites
+// CHECK-LABEL: attributes
+// CHECK-NOT: [[USER_FN_ATTR]] = { {{.*noreturn.*}} }
+// CHECK-NOT: [[EXTERN_FN_ATTR]] = { {{.*noreturn.*}} }
Index: clang/test/CodeGen/ubsan-asan-noreturn.c
===
--- /dev/null
+++ clang/test/CodeGen/ubsan-asan-noreturn.c
@@ -0,0 +1,21 @@
+// Ensure compatiblity of UBSan unreachable with ASan in the presence of
+// noreturn functions.
+// RUN: %clang_cc1 -fsanitize=unreachable,address -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+
+void my_longjmp(void) __attribute__((noreturn));
+
+// CHECK-LABEL: define void @calls_noreturn()
+void calls_noreturn() {
+  my_longjmp();
+  // CHECK:  @__asan_handle_no_return{{.*}} !nosanitize
+  // CHECK-NEXT: @my_longjmp(){{[^#]*}}
+  // CHECK:  @__asan_handle_no_return()
+  // CHECK-NEXT: @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
+  // 

[PATCH] D57278: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

In D57278#1374852 , @vsk wrote:

> Is it necessary to remove visitation of 'noreturn' call sites from ASan? I'd 
> expect that to break non-C frontends which emit noreturn calls (rust/swift?).


Good point! I didn't think about other frontends.

Currently we insert all `noreturn` handler calls in the frontend. Since the 
ASan pass still needs to insert handler calls to support other frontends, I 
would like to change it so that clang only inserts calls when it is necessary 
to prevent UBSan incompatibilities. This means that the ASan pass can remain 
completely unchanged.
(Orthogonal issue exposed by the above: In a follow-up I will change the ASan 
pass to not intrument `!nosanitize` calls to get rid of superfluous handler 
calls.)

@eugenis
We were hoping to only insert `noreturn` handler calls in one place, but I am 
not sure this is feasible when we consider alternative frontends. Are you still 
fine with this?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57278



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln abandoned this revision.
yln added a comment.

Created new revision for this change: https://reviews.llvm.org/D57278


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

In D56624#1370579 , @eugenis wrote:

> Maybe the frontend should insert __asan_handle_noreturn whenever ASan is 
> enabled, and then ASan would not care about the attribute? I'd like to avoid 
> having this logic in two places.


+1 for this. @vsk Can you sign off on this design?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Seems as if we reached consensus! :)  I will change the revision to use an 
intrinsic.

Before I start doing that, just one more quick idea:
Would it work if UBsan directly inserts calls to `__asan_handle_no_return` (of 
course only when ASan is requested). Similar to how it inserts calls to it's 
own runtime functions (e.g., `__ubsan_handle_builtin_unreachable`).
If we strive for the "simplest" solution... but maybe I am missing something in 
this is too simple?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Note that all of this currently only matters when compiling with 
`-fsanitize=unreachable`. The following discussion is within the context of the 
current implementation: UBSan removes the `noreturn` so it can instrument 
`unreachable` without the added instrumentation being optimized away. Maybe we 
should take a step back and ask if that is the right approach at all?

In D56624#1369795 , @vsk wrote:

> Because "expect_noreturn" calls are allowed to return, the compiler must 
> behave as they could. In particular, this means that unpoisoning the stack 
> before expect_noreturn calls (given the current semantics) is premature.
>
> Put another way, a frontend author may (understandably, but mistakenly!) 
> attach expect_noreturn to calls which they expect to be cold.


I think about this differently. Yes, most noreturn functions are also cold, 
e.g., `abort`, but not necessarily, e.g., calls to `longjmp` do not necessarily 
have to be. Why would it be okay to attach expect_noreturn instead of cold? Why 
do we think that this is an easy-to-make mistake? Have people accidentally put 
noreturn on cold functions before?
Can we agree on the following?
"It is orthogonal on the language level, but seems to be redundant in terms of 
the optimizer. Since LLVM IR's main purpose it support the optimizer, this is a 
strong argument against the general purpose attribute."

> That would regress ASan coverage.

You talk specifically about cases of misuses of the attribute, right?
In the context of the current issue with UBSan the possibility for false 
negative is not too much of a regression: it only occurs when UBSan is going to 
diagnose an "unreachable error" anyways.

So the main point is whether or not to use a "general purpose" attribute or a 
"narrow purpose" attribute/intrinsic. My understanding is that you list the 
following points as arguments against general purpose. Is my understanding 
accurate?

1. Potential misuse can regress ASan coverage
2. Complicates optimizer

Narrow purpose: No potential misuses, and optimizer can simply ignore it.

Initially I proposed a narrow purpose attribute, but while iterating on this 
revision changed it to be general purpose. @eugenis 
Now, I have a slight preference for general purpose: I don't think 1. is a big 
issue (but then again, I have no experience here),  and 2. it is always correct 
for the optimizer to continue ignoring the attribute (current status).
Actually, 2. also encompasses the potential upside: a more complicated 
optimizer that takes advantage of the attribute to do additional optimizations.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

@lebedev.ri
Thanks for the clarifications!
I will split this up into multiple patches once we settled on a design.

In D56624#1369635 , @vsk wrote:

> What are the advantages of a generalized expect_noreturn attribute, vs. a 
> narrower attribute or intrinsic? The expect_noreturn semantics do not provide 
> strong guarantees, and are not really orthogonal from the pre-existing cold 
> attribute.


@eugenis Do you want to chime in here?
I think they convey different meanings even if their treatment by the optimizer 
is similar. The `cold` attribute says nothing about whether or not a function 
is expected to return.

> In particular, expect_noreturn doesn't even seem strong enough to allow ASan 
> to unpoison its stack.

I am not sure I understand this part. Can you elaborate?

For context:

  ``cold``
This attribute indicates that this function is rarely called. When
computing edge weights, basic blocks post-dominated by a cold
function call are also considered to be cold; and, thus, given low
weight.
  ``noreturn``
This function attribute indicates that the function never returns
normally. This produces undefined behavior at runtime if the
function ever does dynamically return.
  ``expect_noreturn``
This function attribute indicates that the function is unlikely to return
normally, but that it is still allowed to do so. This is useful in cases
for which ``noreturn`` is too strong a guarantee.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln reopened this revision.
yln added a comment.

In D56624#1368966 , @lebedev.ri wrote:

> Please revert this.
>  First, this wasn't reviewed.
>  Second, the lists weren't subscribed.


I apologize for this. It was not my intention to land the revision without 
formal acceptance.

commit-lists: 
I prepared this patch via the monorepo and did not select a repository in 
Phabricator because the changes span multiple repos. This means that I have to 
manually ensure that the correct lists are subscribed in the Phabricator web 
interface, correct?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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