[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl 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 rG6f026ff02985: Discussion: Darwin Sanitizers Stable ABI 
(authored by rsundahl).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize-stable-abi.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/ASanABI.rst
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/asan_abi_tbd.txt
  compiler-rt/test/asan_abi/CMakeLists.txt
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
  compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp
  compiler-rt/test/asan_abi/lit.cfg.py
  compiler-rt/test/asan_abi/lit.site.cfg.py.in

Index: compiler-rt/test/asan_abi/lit.site.cfg.py.in
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.site.cfg.py.in
@@ -0,0 +1,18 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.name_suffix = "@ASAN_ABI_TEST_CONFIG_SUFFIX@"
+config.target_cflags = "@ASAN_ABI_TEST_TARGET_CFLAGS@"
+config.clang = "@ASAN_ABI_TEST_TARGET_CC@"
+config.bits = "@ASAN_ABI_TEST_BITS@"
+config.arm_thumb = "@COMPILER_RT_ARM_THUMB@"
+config.apple_platform = "@ASAN_ABI_TEST_APPLE_PLATFORM@"
+config.apple_platform_min_deployment_target_flag = "@ASAN_ABI_TEST_MIN_DEPLOYMENT_TARGET_FLAG@"
+config.asan_abi_dynamic = @ASAN_ABI_TEST_DYNAMIC@
+config.target_arch = "@ASAN_ABI_TEST_TARGET_ARCH@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@ASAN_ABI_LIT_SOURCE_DIR@/lit.cfg.py")
Index: compiler-rt/test/asan_abi/lit.cfg.py
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.cfg.py
@@ -0,0 +1,74 @@
+# -*- Python -*-
+
+import os
+import platform
+import re
+
+import lit.formats
+
+def get_required_attr(config, attr_name):
+  attr_value = getattr(config, attr_name, None)
+  if attr_value is None:
+lit_config.fatal(
+  'No attribute %r in test configuration! You may need to run '
+  'tests from your build directory or add this attribute '
+  'to lit.site.cfg.py ' % attr_name)
+  return attr_value
+
+# Setup config name.
+config.name = 'AddressSanitizerABI' + config.name_suffix
+
+# Platform-specific default ASAN_ABI_OPTIONS for lit tests.
+default_asan_abi_opts = list(config.default_sanitizer_opts)
+
+default_asan_abi_opts_str = ':'.join(default_asan_abi_opts)
+if default_asan_abi_opts_str:
+  config.environment['ASAN_ABI_OPTIONS'] = default_asan_abi_opts_str
+  default_asan_abi_opts_str += ':'
+config.substitutions.append(('%env_asan_abi_opts=',
+ 'env ASAN_ABI_OPTIONS=' + default_asan_abi_opts_str))
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# GCC-ASan doesn't link in all the necessary libraries automatically, so
+# we have to do it ourselves.
+extra_link_flags = []
+
+# Setup default compiler flags used with -fsanitize=address option.
+# FIXME: Review the set of required flags and check if it can be reduced.
+target_cflags = [get_required_attr(config, 'target_cflags')] + extra_link_flags
+target_cxxflags = config.cxx_mode_flags + target_cflags
+clang_asan_abi_static_cflags = (['-fsanitize=address',
+'-fsanitize-stable-abi',
+'-mno-omit-leaf-frame-pointer',
+'-fno-omit-frame-pointer',
+'-fno-optimize-sibling-calls'] +
+config.debug_info_flags + target_cflags)
+clang_asan_abi_static_cxxflags = config.cxx_mode_flags + clang_asan_abi_static_cflags
+
+config.available_features.add('asan_abi-static-runtime')
+clang_asan_abi_cflags = clang_asan_abi_static_cflags
+clang_asan_abi_cxxflags = clang_asan_abi_static_cxxflags
+
+def build_invocation(compile_flags):
+  return ' ' + ' '.join([config.clang] + compile_flags) + ' '
+
+config.substitutions.append( ('%clang ', build_invocation(target_cflags)) )
+config.substitutions.append( ('%clangxx ', build_invocation(target_cxxflags)) )
+config.substitutions.append( ('%clang_asan_abi ', build_invocation(clang_asan_abi_cflags)) )
+config.substitutions.append( ('%clangxx_asan_abi ', build_invocation(clang_asan_abi_cxxflags)) )
+
+libasan_abi_path = os.path.join(config.compiler_rt_libdir, 'libclang_rt.asan_abi_osx.a'.format(config.apple_platform))
+
+if libasan_abi_path is not None:
+  

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

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

Thanks for your time and guidance getting this landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 525644.
rsundahl added a comment.

Apply proper backticks quotes to options. Remove redundant ellipses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize-stable-abi.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/ASanABI.rst
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/asan_abi_tbd.txt
  compiler-rt/test/asan_abi/CMakeLists.txt
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
  compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp
  compiler-rt/test/asan_abi/lit.cfg.py
  compiler-rt/test/asan_abi/lit.site.cfg.py.in

Index: compiler-rt/test/asan_abi/lit.site.cfg.py.in
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.site.cfg.py.in
@@ -0,0 +1,18 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.name_suffix = "@ASAN_ABI_TEST_CONFIG_SUFFIX@"
+config.target_cflags = "@ASAN_ABI_TEST_TARGET_CFLAGS@"
+config.clang = "@ASAN_ABI_TEST_TARGET_CC@"
+config.bits = "@ASAN_ABI_TEST_BITS@"
+config.arm_thumb = "@COMPILER_RT_ARM_THUMB@"
+config.apple_platform = "@ASAN_ABI_TEST_APPLE_PLATFORM@"
+config.apple_platform_min_deployment_target_flag = "@ASAN_ABI_TEST_MIN_DEPLOYMENT_TARGET_FLAG@"
+config.asan_abi_dynamic = @ASAN_ABI_TEST_DYNAMIC@
+config.target_arch = "@ASAN_ABI_TEST_TARGET_ARCH@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@ASAN_ABI_LIT_SOURCE_DIR@/lit.cfg.py")
Index: compiler-rt/test/asan_abi/lit.cfg.py
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.cfg.py
@@ -0,0 +1,74 @@
+# -*- Python -*-
+
+import os
+import platform
+import re
+
+import lit.formats
+
+def get_required_attr(config, attr_name):
+  attr_value = getattr(config, attr_name, None)
+  if attr_value is None:
+lit_config.fatal(
+  'No attribute %r in test configuration! You may need to run '
+  'tests from your build directory or add this attribute '
+  'to lit.site.cfg.py ' % attr_name)
+  return attr_value
+
+# Setup config name.
+config.name = 'AddressSanitizerABI' + config.name_suffix
+
+# Platform-specific default ASAN_ABI_OPTIONS for lit tests.
+default_asan_abi_opts = list(config.default_sanitizer_opts)
+
+default_asan_abi_opts_str = ':'.join(default_asan_abi_opts)
+if default_asan_abi_opts_str:
+  config.environment['ASAN_ABI_OPTIONS'] = default_asan_abi_opts_str
+  default_asan_abi_opts_str += ':'
+config.substitutions.append(('%env_asan_abi_opts=',
+ 'env ASAN_ABI_OPTIONS=' + default_asan_abi_opts_str))
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# GCC-ASan doesn't link in all the necessary libraries automatically, so
+# we have to do it ourselves.
+extra_link_flags = []
+
+# Setup default compiler flags used with -fsanitize=address option.
+# FIXME: Review the set of required flags and check if it can be reduced.
+target_cflags = [get_required_attr(config, 'target_cflags')] + extra_link_flags
+target_cxxflags = config.cxx_mode_flags + target_cflags
+clang_asan_abi_static_cflags = (['-fsanitize=address',
+'-fsanitize-stable-abi',
+'-mno-omit-leaf-frame-pointer',
+'-fno-omit-frame-pointer',
+'-fno-optimize-sibling-calls'] +
+config.debug_info_flags + target_cflags)
+clang_asan_abi_static_cxxflags = config.cxx_mode_flags + clang_asan_abi_static_cflags
+
+config.available_features.add('asan_abi-static-runtime')
+clang_asan_abi_cflags = clang_asan_abi_static_cflags
+clang_asan_abi_cxxflags = clang_asan_abi_static_cxxflags
+
+def build_invocation(compile_flags):
+  return ' ' + ' '.join([config.clang] + compile_flags) + ' '
+
+config.substitutions.append( ('%clang ', build_invocation(target_cflags)) )
+config.substitutions.append( ('%clangxx ', build_invocation(target_cxxflags)) )
+config.substitutions.append( ('%clang_asan_abi ', build_invocation(clang_asan_abi_cflags)) )
+config.substitutions.append( ('%clangxx_asan_abi ', build_invocation(clang_asan_abi_cxxflags)) )
+
+libasan_abi_path = os.path.join(config.compiler_rt_libdir, 'libclang_rt.asan_abi_osx.a'.format(config.apple_platform))
+
+if libasan_abi_path is not None:
+  config.substitutions.append( ('%libasan_abi', libasan_abi_path) )
+  config.substitutions.append( 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.

LGTM.




Comment at: compiler-rt/docs/ASanABI.rst:15
+
+...
+void __asan_load1(uptr p) { __asan_abi_loadn(p, 1, true); }

Delete `...`. Sample content implies that this is a code fragment and does not 
contain everything, so `...` is redundant.



Comment at: compiler-rt/docs/ASanABI.rst:22
+
+The shim library is only used when -fsanitize-stable-abi is specified in the 
Clang driver and the emitted instrumentation favors runtime calls over inline 
expansion.
+

Quote all compiler driver options with double backsticks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

Hello Egenii,

Thank you for your time and consideration of this PR. Since you last commented, 
@vitalybuka has approved the PR and added @maskray and yourself as blocking 
reviewers. @maskray has approved and we are awaiting your approval if you 
remain positive to it (or let us know if you have any new reservations).

Thank you,
-Roy Sundahl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 523409.
rsundahl added a comment.

Implement suggestions from latest review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize-stable-abi.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/ASanABI.rst
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/asan_abi_tbd.txt
  compiler-rt/test/asan_abi/CMakeLists.txt
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
  compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp
  compiler-rt/test/asan_abi/lit.cfg.py
  compiler-rt/test/asan_abi/lit.site.cfg.py.in

Index: compiler-rt/test/asan_abi/lit.site.cfg.py.in
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.site.cfg.py.in
@@ -0,0 +1,18 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.name_suffix = "@ASAN_ABI_TEST_CONFIG_SUFFIX@"
+config.target_cflags = "@ASAN_ABI_TEST_TARGET_CFLAGS@"
+config.clang = "@ASAN_ABI_TEST_TARGET_CC@"
+config.bits = "@ASAN_ABI_TEST_BITS@"
+config.arm_thumb = "@COMPILER_RT_ARM_THUMB@"
+config.apple_platform = "@ASAN_ABI_TEST_APPLE_PLATFORM@"
+config.apple_platform_min_deployment_target_flag = "@ASAN_ABI_TEST_MIN_DEPLOYMENT_TARGET_FLAG@"
+config.asan_abi_dynamic = @ASAN_ABI_TEST_DYNAMIC@
+config.target_arch = "@ASAN_ABI_TEST_TARGET_ARCH@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@ASAN_ABI_LIT_SOURCE_DIR@/lit.cfg.py")
Index: compiler-rt/test/asan_abi/lit.cfg.py
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.cfg.py
@@ -0,0 +1,74 @@
+# -*- Python -*-
+
+import os
+import platform
+import re
+
+import lit.formats
+
+def get_required_attr(config, attr_name):
+  attr_value = getattr(config, attr_name, None)
+  if attr_value is None:
+lit_config.fatal(
+  'No attribute %r in test configuration! You may need to run '
+  'tests from your build directory or add this attribute '
+  'to lit.site.cfg.py ' % attr_name)
+  return attr_value
+
+# Setup config name.
+config.name = 'AddressSanitizerABI' + config.name_suffix
+
+# Platform-specific default ASAN_ABI_OPTIONS for lit tests.
+default_asan_abi_opts = list(config.default_sanitizer_opts)
+
+default_asan_abi_opts_str = ':'.join(default_asan_abi_opts)
+if default_asan_abi_opts_str:
+  config.environment['ASAN_ABI_OPTIONS'] = default_asan_abi_opts_str
+  default_asan_abi_opts_str += ':'
+config.substitutions.append(('%env_asan_abi_opts=',
+ 'env ASAN_ABI_OPTIONS=' + default_asan_abi_opts_str))
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# GCC-ASan doesn't link in all the necessary libraries automatically, so
+# we have to do it ourselves.
+extra_link_flags = []
+
+# Setup default compiler flags used with -fsanitize=address option.
+# FIXME: Review the set of required flags and check if it can be reduced.
+target_cflags = [get_required_attr(config, 'target_cflags')] + extra_link_flags
+target_cxxflags = config.cxx_mode_flags + target_cflags
+clang_asan_abi_static_cflags = (['-fsanitize=address',
+'-fsanitize-stable-abi',
+'-mno-omit-leaf-frame-pointer',
+'-fno-omit-frame-pointer',
+'-fno-optimize-sibling-calls'] +
+config.debug_info_flags + target_cflags)
+clang_asan_abi_static_cxxflags = config.cxx_mode_flags + clang_asan_abi_static_cflags
+
+config.available_features.add('asan_abi-static-runtime')
+clang_asan_abi_cflags = clang_asan_abi_static_cflags
+clang_asan_abi_cxxflags = clang_asan_abi_static_cxxflags
+
+def build_invocation(compile_flags):
+  return ' ' + ' '.join([config.clang] + compile_flags) + ' '
+
+config.substitutions.append( ('%clang ', build_invocation(target_cflags)) )
+config.substitutions.append( ('%clangxx ', build_invocation(target_cxxflags)) )
+config.substitutions.append( ('%clang_asan_abi ', build_invocation(clang_asan_abi_cflags)) )
+config.substitutions.append( ('%clangxx_asan_abi ', build_invocation(clang_asan_abi_cxxflags)) )
+
+libasan_abi_path = os.path.join(config.compiler_rt_libdir, 'libclang_rt.asan_abi_osx.a'.format(config.apple_platform))
+
+if libasan_abi_path is not None:
+  config.substitutions.append( ('%libasan_abi', libasan_abi_path) )
+  config.substitutions.append( ('%clang_asan_abi_static ', 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

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



Comment at: compiler-rt/docs/ASanABI.rst:29
+
+  Following are some examples of reasonable responses to such changes:
+

MaskRay wrote:
> How does the 2-space indentation render in the built HTML? It may look good, 
> I ask just in case.
> How does the 2-space indentation render in the built HTML? It may look good, 
> I ask just in case.

IDK so I played around with it. Global search/replace 2 spaces with 4 does not 
affect rendering of the block above at all. In the block below there was an 
effect which was to indent the code blocks one more stop. The reason for this 
seems to be that the "current indent level" is advanced by 2 in a bulleted 
list, so below, the code block statement is actually aligned with the bulleted 
paragraph above it. After the GSR, the code block is indented and rendered 
further indented. Since bullets advance the "current indent level" by 2, maybe 
a more natural indent for the source code (.rst) is to use 2 as well. Seems to 
read a little easier in the source and avoids having to think about all the 
multiple of 4's after a bullet being "off by 2".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: compiler-rt/docs/ASanABI.rst:29
+
+  Following are some examples of reasonable responses to such changes:
+

How does the 2-space indentation render in the built HTML? It may look good, I 
ask just in case.



Comment at: compiler-rt/lib/asan_abi/asan_abi.h:18
+// Functions concerning instrumented global variables:
+void __asan_abi_register_image_globals(void);
+void __asan_abi_unregister_image_globals(void);

`asan_abi.h` is C++ only (`extern "C"` isn't allowed in C). `(void)` should be 
replaced with `()`.



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:14
+extern "C" {
+// Functions concerning instrumented global variables:
+void __asan_register_image_globals(uptr *flag) {

Below there is no `:`. You may omit this `:` as well.



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:345
+// Functions concerning fake stacks
+void *__asan_get_current_fake_stack(void) {
+  // TBD: Fail here





Comment at: compiler-rt/test/asan_abi/lit.cfg.py:13
+lit_config.fatal(
+  "No attribute %r in test configuration! You may need to run "
+  "tests from your build directory or add this attribute "

This file mixes single quotes and double quotes (the file it copied from does 
so as well). Pick one and be consistent!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

Ping @kcc , @yln and @kubamracek


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

@MaskRay, thank you for your approval. @eugenis, you were added as a blocking 
reviewer by @vitalybuka. If you are still without objection, can we get your 
approval and merge? Thank you all for your input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 522592.
rsundahl added a comment.

Renamed darwin_exclude_symbols.inc asan_abi_tbd.txt.

This file contains the entrypoints that aren't strictly in the interface
between the instrumentation and the runtime but may still be part of a
public API that needs to have a home in Stable ABI. For now we acknowledge
them in the existing ABI and explicitly list them as TBD until we have a
complete story for how the shim deals with them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize-stable-abi.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/ASanABI.rst
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/asan_abi_tbd.txt
  compiler-rt/test/asan_abi/CMakeLists.txt
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
  compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp
  compiler-rt/test/asan_abi/lit.cfg.py
  compiler-rt/test/asan_abi/lit.site.cfg.py.in

Index: compiler-rt/test/asan_abi/lit.site.cfg.py.in
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.site.cfg.py.in
@@ -0,0 +1,18 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.name_suffix = "@ASAN_ABI_TEST_CONFIG_SUFFIX@"
+config.target_cflags = "@ASAN_ABI_TEST_TARGET_CFLAGS@"
+config.clang = "@ASAN_ABI_TEST_TARGET_CC@"
+config.bits = "@ASAN_ABI_TEST_BITS@"
+config.arm_thumb = "@COMPILER_RT_ARM_THUMB@"
+config.apple_platform = "@ASAN_ABI_TEST_APPLE_PLATFORM@"
+config.apple_platform_min_deployment_target_flag = "@ASAN_ABI_TEST_MIN_DEPLOYMENT_TARGET_FLAG@"
+config.asan_abi_dynamic = @ASAN_ABI_TEST_DYNAMIC@
+config.target_arch = "@ASAN_ABI_TEST_TARGET_ARCH@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@ASAN_ABI_LIT_SOURCE_DIR@/lit.cfg.py")
Index: compiler-rt/test/asan_abi/lit.cfg.py
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.cfg.py
@@ -0,0 +1,74 @@
+# -*- Python -*-
+
+import os
+import platform
+import re
+
+import lit.formats
+
+def get_required_attr(config, attr_name):
+  attr_value = getattr(config, attr_name, None)
+  if attr_value is None:
+lit_config.fatal(
+  "No attribute %r in test configuration! You may need to run "
+  "tests from your build directory or add this attribute "
+  "to lit.site.cfg.py " % attr_name)
+  return attr_value
+
+# Setup config name.
+config.name = 'AddressSanitizerABI' + config.name_suffix
+
+# Platform-specific default ASAN_ABI_OPTIONS for lit tests.
+default_asan_abi_opts = list(config.default_sanitizer_opts)
+
+default_asan_abi_opts_str = ':'.join(default_asan_abi_opts)
+if default_asan_abi_opts_str:
+  config.environment['ASAN_ABI_OPTIONS'] = default_asan_abi_opts_str
+  default_asan_abi_opts_str += ':'
+config.substitutions.append(('%env_asan_abi_opts=',
+ 'env ASAN_ABI_OPTIONS=' + default_asan_abi_opts_str))
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# GCC-ASan doesn't link in all the necessary libraries automatically, so
+# we have to do it ourselves.
+extra_link_flags = []
+
+# Setup default compiler flags used with -fsanitize=address option.
+# FIXME: Review the set of required flags and check if it can be reduced.
+target_cflags = [get_required_attr(config, "target_cflags")] + extra_link_flags
+target_cxxflags = config.cxx_mode_flags + target_cflags
+clang_asan_abi_static_cflags = (["-fsanitize=address",
+"-fsanitize-stable-abi",
+"-mno-omit-leaf-frame-pointer",
+"-fno-omit-frame-pointer",
+"-fno-optimize-sibling-calls"] +
+config.debug_info_flags + target_cflags)
+clang_asan_abi_static_cxxflags = config.cxx_mode_flags + clang_asan_abi_static_cflags
+
+config.available_features.add("asan_abi-static-runtime")
+clang_asan_abi_cflags = clang_asan_abi_static_cflags
+clang_asan_abi_cxxflags = clang_asan_abi_static_cxxflags
+
+def build_invocation(compile_flags):
+  return " " + " ".join([config.clang] + compile_flags) + " "
+
+config.substitutions.append( ("%clang ", build_invocation(target_cflags)) )
+config.substitutions.append( ("%clangxx ", build_invocation(target_cxxflags)) )
+config.substitutions.append( ("%clang_asan_abi ", build_invocation(clang_asan_abi_cflags)) )
+config.substitutions.append( 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 522452.
rsundahl added a comment.

Missed one file in revert of combined -mllvm= change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize-stable-abi.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/ASanABI.rst
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc
  compiler-rt/test/asan_abi/CMakeLists.txt
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
  compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp
  compiler-rt/test/asan_abi/lit.cfg.py
  compiler-rt/test/asan_abi/lit.site.cfg.py.in

Index: compiler-rt/test/asan_abi/lit.site.cfg.py.in
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.site.cfg.py.in
@@ -0,0 +1,18 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.name_suffix = "@ASAN_ABI_TEST_CONFIG_SUFFIX@"
+config.target_cflags = "@ASAN_ABI_TEST_TARGET_CFLAGS@"
+config.clang = "@ASAN_ABI_TEST_TARGET_CC@"
+config.bits = "@ASAN_ABI_TEST_BITS@"
+config.arm_thumb = "@COMPILER_RT_ARM_THUMB@"
+config.apple_platform = "@ASAN_ABI_TEST_APPLE_PLATFORM@"
+config.apple_platform_min_deployment_target_flag = "@ASAN_ABI_TEST_MIN_DEPLOYMENT_TARGET_FLAG@"
+config.asan_abi_dynamic = @ASAN_ABI_TEST_DYNAMIC@
+config.target_arch = "@ASAN_ABI_TEST_TARGET_ARCH@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@ASAN_ABI_LIT_SOURCE_DIR@/lit.cfg.py")
Index: compiler-rt/test/asan_abi/lit.cfg.py
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.cfg.py
@@ -0,0 +1,74 @@
+# -*- Python -*-
+
+import os
+import platform
+import re
+
+import lit.formats
+
+def get_required_attr(config, attr_name):
+  attr_value = getattr(config, attr_name, None)
+  if attr_value is None:
+lit_config.fatal(
+  "No attribute %r in test configuration! You may need to run "
+  "tests from your build directory or add this attribute "
+  "to lit.site.cfg.py " % attr_name)
+  return attr_value
+
+# Setup config name.
+config.name = 'AddressSanitizerABI' + config.name_suffix
+
+# Platform-specific default ASAN_ABI_OPTIONS for lit tests.
+default_asan_abi_opts = list(config.default_sanitizer_opts)
+
+default_asan_abi_opts_str = ':'.join(default_asan_abi_opts)
+if default_asan_abi_opts_str:
+  config.environment['ASAN_ABI_OPTIONS'] = default_asan_abi_opts_str
+  default_asan_abi_opts_str += ':'
+config.substitutions.append(('%env_asan_abi_opts=',
+ 'env ASAN_ABI_OPTIONS=' + default_asan_abi_opts_str))
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# GCC-ASan doesn't link in all the necessary libraries automatically, so
+# we have to do it ourselves.
+extra_link_flags = []
+
+# Setup default compiler flags used with -fsanitize=address option.
+# FIXME: Review the set of required flags and check if it can be reduced.
+target_cflags = [get_required_attr(config, "target_cflags")] + extra_link_flags
+target_cxxflags = config.cxx_mode_flags + target_cflags
+clang_asan_abi_static_cflags = (["-fsanitize=address",
+"-fsanitize-stable-abi",
+"-mno-omit-leaf-frame-pointer",
+"-fno-omit-frame-pointer",
+"-fno-optimize-sibling-calls"] +
+config.debug_info_flags + target_cflags)
+clang_asan_abi_static_cxxflags = config.cxx_mode_flags + clang_asan_abi_static_cflags
+
+config.available_features.add("asan_abi-static-runtime")
+clang_asan_abi_cflags = clang_asan_abi_static_cflags
+clang_asan_abi_cxxflags = clang_asan_abi_static_cxxflags
+
+def build_invocation(compile_flags):
+  return " " + " ".join([config.clang] + compile_flags) + " "
+
+config.substitutions.append( ("%clang ", build_invocation(target_cflags)) )
+config.substitutions.append( ("%clangxx ", build_invocation(target_cxxflags)) )
+config.substitutions.append( ("%clang_asan_abi ", build_invocation(clang_asan_abi_cflags)) )
+config.substitutions.append( ("%clangxx_asan_abi ", build_invocation(clang_asan_abi_cxxflags)) )
+
+libasan_abi_path = os.path.join(config.compiler_rt_libdir, 'libclang_rt.asan_abi_osx.a'.format(config.apple_platform))
+
+if libasan_abi_path is not None:
+  config.substitutions.append( ("%libasan_abi", libasan_abi_path) )
+  config.substitutions.append( 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

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

Suggestions for compiler-rt/docs/asan_abi.md are captured in the successor file 
compiler-rt/docs/ASanABI.rst and marked complete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl marked 4 inline comments as done.
rsundahl added a comment.

Suggestions for compiler-rt/docs/asan_abi.md are captured in the successor file 
compiler-rt/docs/ASanABI.rst and marked complete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl marked 22 inline comments as done.
rsundahl added a comment.

Thank you for your review and thoughtful input @eugenis, @MaskRay and 
@vitalybuka. I think we're close to having everything incorporated. (@MaskRay, 
the doc files went from .md to .rst and I implemented all of your suggestions 
there.




Comment at: clang/include/clang/Driver/Options.td:1785
HelpText<"Use default code 
inlining logic for the address sanitizer">;
+def fsanitize_address_stable_abi : Flag<["-"], "fsanitize-address-stable-abi">,
+Group,

MaskRay wrote:
> rsundahl wrote:
> > vitalybuka wrote:
> > > how likely you will need thus for  other sanitizers in future
> > > should this be rather -fsanitize-stable-abi which is ignore for now for 
> > > other sanitizers?
> > Thanks @vitalybuka. I agree and made this change. For now I still only 
> > consume the flag if sanitizer=address so that we continue to get the unused 
> > option warning in the other cases and left the test for that warning intact.
> See `BooleanFFlag`. Some existing sanitizer options are not written with the 
> best practice.
> 
> If `-fno-sanitize-stable-abi` is the default, there is usually no need to 
> have a duplicate help message `Disable ...`. Documenting the non-default 
> boolean option suffices.
(Not sure if this is exactly what you meant @MaskRay but I think it's close.)



Comment at: clang/lib/Driver/SanitizerArgs.cpp:1292
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-asan-instrumentation-with-call-threshold=0");
+CmdArgs.push_back("-mllvm");

MaskRay wrote:
> Optional nit: I added `-mllvm=` as an alias in `D143325`. You can use 
> `-mllvm=-asan-instrumentation-with-call-threshold=0` to decrease the 
> number/length of cc1 options.
> 
> Add some comments before `if (StableABI) {` why the two `cl::opt` options are 
> specified.
I couldn't get this one to work. Did I do it wrong? (Couldn't find example in 
the code to go from.)

Tried:
```
  if (StableABI) {
CmdArgs.push_back("-mllvm=-asan-instrumentation-with-call-threshold=0");
CmdArgs.push_back("-mllvm=-asan-max-inline-poisoning-size=0");
  }
```
Got:
```
error: unknown argument: '-mllvm=-asan-instrumentation-with-call-threshold=0'
error: unknown argument: '-mllvm=-asan-max-inline-poisoning-size=0'
```



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:41
+
+void __asan_after_dynamic_init(void) {
+__asan_abi_after_dynamic_init();

MaskRay wrote:
> C++ prefers `()` instead of `(void)`.
These are actually "C'
Added:
```
extern "C" {
...
}
```



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:486
+// TBD: Fail here
+return (void *) 0;
+}

MaskRay wrote:
> You may apply `clang-format`, which may turn this into `(void *)0`, but 
> `nullptr` likely works better.
clang-format left this as-is. I suspect this is because I also added the extern 
"C" brackets.



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:16
+//
+// RUN:  sed -e ':a' -e 'N' -e '$!ba'  
   \
+// RUN:  -e 's/ //g'   
   \

MaskRay wrote:
> Does Darwin sed accept `;` to combine multiple `-e` into one `-e` with `;`?
I couldn't get the same behavior out of the intersection of GNU an BSD set. 
Tried hard in https://reviews.llvm.org/D138824 and landed with the -e's. iirc 
exactly what the problem was with semicolons, just that I was relieved when I 
found a format that worked for all the platforms.



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:26
+//
+// RUN: cat %t.imports | sort | uniq > %t.imports-sorted
+// RUN: cat %t.exports | sort | uniq > %t.exports-sorted

MaskRay wrote:
> `sort %t.imports`. See `Useless use of cat`
Good point!



Comment at: compiler-rt/test/asan_abi/lit.cfg.py:9
+
+# Get shlex.quote if available (added in 3.3), and fall back to pipes.quote if
+# it's not available.

MaskRay wrote:
> This workaround is unneeded. I sent D150410 to clean up other `lit.cfg.py` 
> files.
Wasn't actually used anyway but good to know!



Comment at: compiler-rt/test/asan_abi/lit.cfg.py:83
+# Only run the tests on supported OSs.
+if config.host_os not in ['Darwin']:
+  config.unsupported = True

thetruestblue wrote:
> MaskRay wrote:
> > `!=`
> The thought here was to leave basic lit patterns in-tact to expand to other 
> OSs if others want to in the future. But if there's no desire for that, it 
> doesn't make a big difference to me. 
I landed on just an else clause. Let me know if that's ok @thetruestblue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 522417.
rsundahl added a comment.

Applied suggestions from reviewers

Cleaned up options parsing
Moved test into stanalone file fsanitize-stable-abi.c
Changed target triple to arm64-apple-darwin
Changed documentation style from proposal to specification
Changed format of documentation form .md to .rst
Applied clang-format to entire diff
Removed extraneous spaces and lines
Expanded comments to sentences
Switched to static_assert()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize-stable-abi.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/ASanABI.rst
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc
  compiler-rt/test/asan_abi/CMakeLists.txt
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
  compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp
  compiler-rt/test/asan_abi/lit.cfg.py
  compiler-rt/test/asan_abi/lit.site.cfg.py.in

Index: compiler-rt/test/asan_abi/lit.site.cfg.py.in
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.site.cfg.py.in
@@ -0,0 +1,18 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.name_suffix = "@ASAN_ABI_TEST_CONFIG_SUFFIX@"
+config.target_cflags = "@ASAN_ABI_TEST_TARGET_CFLAGS@"
+config.clang = "@ASAN_ABI_TEST_TARGET_CC@"
+config.bits = "@ASAN_ABI_TEST_BITS@"
+config.arm_thumb = "@COMPILER_RT_ARM_THUMB@"
+config.apple_platform = "@ASAN_ABI_TEST_APPLE_PLATFORM@"
+config.apple_platform_min_deployment_target_flag = "@ASAN_ABI_TEST_MIN_DEPLOYMENT_TARGET_FLAG@"
+config.asan_abi_dynamic = @ASAN_ABI_TEST_DYNAMIC@
+config.target_arch = "@ASAN_ABI_TEST_TARGET_ARCH@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@ASAN_ABI_LIT_SOURCE_DIR@/lit.cfg.py")
Index: compiler-rt/test/asan_abi/lit.cfg.py
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.cfg.py
@@ -0,0 +1,74 @@
+# -*- Python -*-
+
+import os
+import platform
+import re
+
+import lit.formats
+
+def get_required_attr(config, attr_name):
+  attr_value = getattr(config, attr_name, None)
+  if attr_value is None:
+lit_config.fatal(
+  "No attribute %r in test configuration! You may need to run "
+  "tests from your build directory or add this attribute "
+  "to lit.site.cfg.py " % attr_name)
+  return attr_value
+
+# Setup config name.
+config.name = 'AddressSanitizerABI' + config.name_suffix
+
+# Platform-specific default ASAN_ABI_OPTIONS for lit tests.
+default_asan_abi_opts = list(config.default_sanitizer_opts)
+
+default_asan_abi_opts_str = ':'.join(default_asan_abi_opts)
+if default_asan_abi_opts_str:
+  config.environment['ASAN_ABI_OPTIONS'] = default_asan_abi_opts_str
+  default_asan_abi_opts_str += ':'
+config.substitutions.append(('%env_asan_abi_opts=',
+ 'env ASAN_ABI_OPTIONS=' + default_asan_abi_opts_str))
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# GCC-ASan doesn't link in all the necessary libraries automatically, so
+# we have to do it ourselves.
+extra_link_flags = []
+
+# Setup default compiler flags used with -fsanitize=address option.
+# FIXME: Review the set of required flags and check if it can be reduced.
+target_cflags = [get_required_attr(config, "target_cflags")] + extra_link_flags
+target_cxxflags = config.cxx_mode_flags + target_cflags
+clang_asan_abi_static_cflags = (["-fsanitize=address",
+"-fsanitize-stable-abi",
+"-mno-omit-leaf-frame-pointer",
+"-fno-omit-frame-pointer",
+"-fno-optimize-sibling-calls"] +
+config.debug_info_flags + target_cflags)
+clang_asan_abi_static_cxxflags = config.cxx_mode_flags + clang_asan_abi_static_cflags
+
+config.available_features.add("asan_abi-static-runtime")
+clang_asan_abi_cflags = clang_asan_abi_static_cflags
+clang_asan_abi_cxxflags = clang_asan_abi_static_cxxflags
+
+def build_invocation(compile_flags):
+  return " " + " ".join([config.clang] + compile_flags) + " "
+
+config.substitutions.append( ("%clang ", build_invocation(target_cflags)) )
+config.substitutions.append( ("%clangxx ", build_invocation(target_cxxflags)) )
+config.substitutions.append( ("%clang_asan_abi ", build_invocation(clang_asan_abi_cflags)) )

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Brittany Blue Gaston via Phabricator via cfe-commits]

thetruestblue added inline comments.



Comment at: compiler-rt/test/asan_abi/lit.cfg.py:83
+# Only run the tests on supported OSs.
+if config.host_os not in ['Darwin']:
+  config.unsupported = True

MaskRay wrote:
> `!=`
The thought here was to leave basic lit patterns in-tact to expand to other OSs 
if others want to in the future. But if there's no desire for that, it doesn't 
make a big difference to me. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D143675#4310904 , @vitalybuka 
wrote:

> In D143675#4310734 , @rsundahl 
> wrote:
>
>> @kcc @eugenis @MaskRay @vitalybuka Ok to go with this? All new functionality 
>> is under the added flag so not expecting any surprises. Rename 
>> asabi->asan_abi as suggested.
>
> Thanks, asan_abi LGTM.
>
> I don't have good reasons to object that patch, but I suspect it's 
> sub-optimal. But we may get a valuable expirience.
>
>> Rather than adding a lot of conditional code to the LLVM instrumentation 
>> phase
>
> We do this for hwasan for Android, and to some extent msan for Chromium. 
> @eugenis maybe can share more info.
>
>> Based on previous discussions about this topic, our understanding is that 
>> freezing the present ABI would impose an excessive burden on other sanitizer 
>> developers and for unrelated platforms.
>
> I guess we just have no way to enforce that. A couple of buildbots with 
> "stable clang" + "HEAD runtime" and "HEAD clang" + "stable runtime" which do 
> some non-tivial build, e.g. clang bootstrap can enforce that. We can at list 
> to enforce default set of flags.

Very sorry for my belated response. I feel that I am not a decision maker, so I 
am waiting on the maintainers.
I do care about driver options (as a code owner) and sanitizer maintainability 
(my interest), though. I have left some comments and will be happy when they 
are resolved.

The documentation `compiler-rt/docs/asan_abi.md` probably needs more polishing. 
The current style is like seeking for RFC, not for something already in tree.




Comment at: compiler-rt/docs/asan_abi.md:3
+
+We wish to make it possible to include the AddressSanitizer (ASan) runtime 
implementation in OSes and for this we need a stable ASan ABI. Based on 
previous discussions about this topic, our understanding is that freezing the 
present ABI would impose an excessive burden on other sanitizer developers and 
for unrelated platforms. Therefore, we propose adding a secondary stable ABI 
for our use and anyone else in the community seeking the same. We believe that 
we can define a stable ABI with minimal burden on the community, expecting only 
to keep existing tests running and implementing stubs when new features are 
added. We are okay with trading performance for stability with no impact for 
existing users of ASan while minimizing the maintenance burden for ASan 
maintainers. We wish to commit this functionality to the LLVM project to 
maintain it there. This new and stable ABI will abstract away the 
implementation details allowing new and novel approaches to ASan for 
developers, researchers and others.
+

The introductory paragraph is written in a style like the RFC, but not for the 
official documentation.
The official documentation should be written in a style that this has been 
accepted.
For sentences like maintenance costs, they can be moved to the `Maintenance` 
chapter.

I have an simplified introductory paragraph:

> Some OSes like Darwin want to include the AddressSanitizer runtime by 
> establishing a stable ASan ABI. `lib/asan_abi` contains a secondary stable 
> ABI for our use and others in the community. This new ABI will have minimal 
> impact on the community, prioritizing stability over performance.

Feel free to add more sentences if you feel too simplified.

Note that `.rst` uses two backsticks for what Markdown uses one backstick for.




Comment at: compiler-rt/docs/asan_abi.md:7
+
+Rather than adding a lot of conditional code to the LLVM instrumentation 
phase, which would incur excessive complexity and maintenance cost of adding 
conditional code into all places that emit a runtime call, we propose a “shim” 
layer which will map the unstable ABI to the stable ABI:
+

Similarly, words like "propose" are RFC-style wording, not for the official 
documentation. For the official documentation, you just say what this is.



Comment at: compiler-rt/docs/asan_abi.md:14
+* `void __asan_poison_cxx_array_cookie(uptr p) { __asan_abi_pac(p); }`
+* This “shim” library would only be used by people who opt in: A compilation 
flag in the Clang driver will be used to gate the use of the stable ABI 
workflow.
+* Utilize the existing ability for the ASan instrumentation to prefer runtime 
calls instead of inlined direct shadow memory accesses.

This “shim” library will only be used when `-fsanitize-stable-abi` is specified 
in the Clang driver.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D143675#4336365 , @rsundahl wrote:

> In D143675#4310903 , @eugenis wrote:
>
>> I'm fine with it in general. Is asan_abi.cpp meant as a temporary stub? It's 
>> not even link anywhere in the current version.
>
> We now use it during testing to close the loop on the question of whether the 
> file is "complete" in the sense that it satisfies the minimal "no-op" 
> implementation of the abi. We also moved from having a hand-curated include 
> file to using the actual interface file which should be the root truth for 
> what needs to be in there. We discovered a few additional functions that were 
> in asan_interface.h but aren't strictly part of the interface between the 
> instrumentation and the runtime but rather are used intra-runtime. Some other 
> routines living in asan_interface.h are really documented "helper" functions. 
> Maybe these should be aggregated somewhere else and/or under a different 
> namespace. For now we ignore those entrypoints by listing them in 
> compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc

How is `compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc` used in the 
upstream and downstream build system?
In this patch this file is only used by one test?




Comment at: clang/include/clang/Driver/Options.td:1785
HelpText<"Use default code 
inlining logic for the address sanitizer">;
+def fsanitize_address_stable_abi : Flag<["-"], "fsanitize-address-stable-abi">,
+Group,

rsundahl wrote:
> vitalybuka wrote:
> > how likely you will need thus for  other sanitizers in future
> > should this be rather -fsanitize-stable-abi which is ignore for now for 
> > other sanitizers?
> Thanks @vitalybuka. I agree and made this change. For now I still only 
> consume the flag if sanitizer=address so that we continue to get the unused 
> option warning in the other cases and left the test for that warning intact.
See `BooleanFFlag`. Some existing sanitizer options are not written with the 
best practice.

If `-fno-sanitize-stable-abi` is the default, there is usually no need to have 
a duplicate help message `Disable ...`. Documenting the non-default boolean 
option suffices.



Comment at: clang/lib/Driver/SanitizerArgs.cpp:918
+options::OPT_fno_sanitize_stable_abi,
+StableABI);
+

Existing code unnecessarily reads the previous value (false) of the variable. 
No need to copy that for new code.



Comment at: clang/lib/Driver/SanitizerArgs.cpp:1292
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-asan-instrumentation-with-call-threshold=0");
+CmdArgs.push_back("-mllvm");

Optional nit: I added `-mllvm=` as an alias in `D143325`. You can use 
`-mllvm=-asan-instrumentation-with-call-threshold=0` to decrease the 
number/length of cc1 options.

Add some comments before `if (StableABI) {` why the two `cl::opt` options are 
specified.



Comment at: clang/test/Driver/fsanitize.c:266
 
+// RUN: %clang --target=x86_64-linux-gnu -fsanitize-stable-abi %s -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=CHECK-ASAN-STABLE-WARN

I think the tests should go to a new file `fsanitize-stable-abi.c`. The checks 
are different enough from the rest of `fsanitize.c` (which can be split).



Comment at: clang/test/Driver/fsanitize.c:269
+// CHECK-ASAN-STABLE-WARN: warning: argument unused during compilation: 
'-fsanitize-stable-abi'
+// RUN: %clang --target=x86_64-linux-gnu -fsanitize=address 
-fsanitize-stable-abi %s -### 2>&1 | \
+// RUN: FileCheck %s --check-prefix=CHECK-ASAN-STABLE-OK

I presume that you want to test the positive forms with Darwin triples like 
`arm64-apple-darwin`?

We can even argue that the option should lead to an error for non-Darwin 
triples.



Comment at: compiler-rt/docs/asan_abi.md:1
+# Darwin Sanitizers Stable ABI
+

The existing compiler-rt/docs docs use `.rst`. Better to use `.rst` to not 
introduce more than one format for one subproject.

`.rst` is used much more than `.md` in llvm-project anyway.



Comment at: compiler-rt/lib/asan_abi/asan_abi.h:13
+#include 
+#include 
+#include 

use clang-format to sort the headers. I'd expect that stdbool and stddef are 
placed together for any sorting behavior.



Comment at: compiler-rt/lib/asan_abi/asan_abi.h:15
+#include 
+extern "C" {
+void __asan_abi_register_image_globals(void);

add a blank line before `extern "C" {`



Comment at: compiler-rt/test/asan_abi/lit.cfg.py:9
+
+# Get shlex.quote if available (added in 3.3), and fall back to pipes.quote if
+# it's not available.

This workaround is unneeded. 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:14
+extern "C" {
+// Globals
+void __asan_register_image_globals(uptr *flag) {

Comments are usually a complete sentence with a period. There are exceptions, 
but a "Globals" needs elaboration to make it better understandable by a reader.



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:16
+void __asan_register_image_globals(uptr *flag) {
+__asan_abi_register_image_globals();
+}

2-space indentation, here and throughout.



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:41
+
+void __asan_after_dynamic_init(void) {
+__asan_abi_after_dynamic_init();

C++ prefers `()` instead of `(void)`.



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:486
+// TBD: Fail here
+return (void *) 0;
+}

You may apply `clang-format`, which may turn this into `(void *)0`, but 
`nullptr` likely works better.



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:1
+// RUN: %clang_asan_abi  -O2 -c -fsanitize-stable-abi -fsanitize=address -O0 
%s -o %t.o
+// RUN: %clangxx -c %p/../../../../lib/asan_abi/asan_abi.cpp -o asan_abi.o

excess spaces

`-O2 ... -O0`?



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:4
+// RUN: %clangxx -dead_strip -o %t %t.o %libasan_abi asan_abi.o && %run %t 2>&1
+
+

One blank line suffices.



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:9
+
+// RUN: nm -g %libasan_abi \
+// RUN:  | grep " [TU] "   \

We generally prefer llvm counterparts to the system binary utilities. Use 
`llvm-nm`



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:15
+// RUN:  > %t.exports
+//
+// RUN:  sed -e ':a' -e 'N' -e '$!ba'  
   \

unneeded `^//$` lines, here and throughout.



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:16
+//
+// RUN:  sed -e ':a' -e 'N' -e '$!ba'  
   \
+// RUN:  -e 's/ //g'   
   \

Does Darwin sed accept `;` to combine multiple `-e` into one `-e` with `;`?



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:21
+// RUN:  %t.asan_interface.inc 
   \
+// RUN:  | grep  -v -f %p/../../../../lib/asan_abi/darwin_exclude_symbols.inc  
   \
+// RUN:  | grep -e "INTERFACE_\(WEAK_\)\?FUNCTION" 
   \

2-space indentation for `|` continuation lines as well



Comment at: 
compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:26
+//
+// RUN: cat %t.imports | sort | uniq > %t.imports-sorted
+// RUN: cat %t.exports | sort | uniq > %t.exports-sorted

`sort %t.imports`. See `Useless use of cat`



Comment at: compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp:1
+// RUN: %clang_asan_abi  -O2 -c -fsanitize-stable-abi -fsanitize=address -O0 
%s -o %t.o
+// RUN: %clangxx -c %p/../../../lib/asan_abi/asan_abi.cpp -o asan_abi.o

`-O2 ... -O0`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:62
+void __asan_init(void) {
+assert(sizeof(uptr) == 8);
+assert(sizeof(u64) == 8);

static_assert


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

In D143675#4310903 , @eugenis wrote:

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

We now use it during testing to close the loop on the question of whether the 
file is "complete" in the sense that it satisfies the minimal "no-op" 
implementation of the abi. We also moved from having a hand-curated include 
file to using the actual interface file which should be the root truth for what 
needs to be in there. We discovered a few additional functions that were in 
asan_interface.h but aren't strictly part of the interface between the 
instrumentation and the runtime but rather are used intra-runtime. Some other 
routines living in asan_interface.h are really documented "helper" functions. 
Maybe these should be aggregated somewhere else and/or under a different 
namespace. For now we ignore those entrypoints by listing them in 
compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 521482.
rsundahl added a comment.

Fixed nits (missing newlines at end of files)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/asan_abi.md
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc
  compiler-rt/test/asan_abi/CMakeLists.txt
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
  compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp
  compiler-rt/test/asan_abi/lit.cfg.py
  compiler-rt/test/asan_abi/lit.site.cfg.py.in

Index: compiler-rt/test/asan_abi/lit.site.cfg.py.in
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.site.cfg.py.in
@@ -0,0 +1,18 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.name_suffix = "@ASAN_ABI_TEST_CONFIG_SUFFIX@"
+config.target_cflags = "@ASAN_ABI_TEST_TARGET_CFLAGS@"
+config.clang = "@ASAN_ABI_TEST_TARGET_CC@"
+config.bits = "@ASAN_ABI_TEST_BITS@"
+config.arm_thumb = "@COMPILER_RT_ARM_THUMB@"
+config.apple_platform = "@ASAN_ABI_TEST_APPLE_PLATFORM@"
+config.apple_platform_min_deployment_target_flag = "@ASAN_ABI_TEST_MIN_DEPLOYMENT_TARGET_FLAG@"
+config.asan_abi_dynamic = @ASAN_ABI_TEST_DYNAMIC@
+config.target_arch = "@ASAN_ABI_TEST_TARGET_ARCH@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@ASAN_ABI_LIT_SOURCE_DIR@/lit.cfg.py")
Index: compiler-rt/test/asan_abi/lit.cfg.py
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.cfg.py
@@ -0,0 +1,84 @@
+# -*- Python -*-
+
+import os
+import platform
+import re
+
+import lit.formats
+
+# Get shlex.quote if available (added in 3.3), and fall back to pipes.quote if
+# it's not available.
+try:
+  import shlex
+  sh_quote = shlex.quote
+except:
+  import pipes
+  sh_quote = pipes.quote
+
+def get_required_attr(config, attr_name):
+  attr_value = getattr(config, attr_name, None)
+  if attr_value == None:
+lit_config.fatal(
+  "No attribute %r in test configuration! You may need to run "
+  "tests from your build directory or add this attribute "
+  "to lit.site.cfg.py " % attr_name)
+  return attr_value
+
+# Setup config name.
+config.name = 'AddressSanitizerABI' + config.name_suffix
+
+# Platform-specific default ASAN_ABI_OPTIONS for lit tests.
+default_asan_abi_opts = list(config.default_sanitizer_opts)
+
+default_asan_abi_opts_str = ':'.join(default_asan_abi_opts)
+if default_asan_abi_opts_str:
+  config.environment['ASAN_ABI_OPTIONS'] = default_asan_abi_opts_str
+  default_asan_abi_opts_str += ':'
+config.substitutions.append(('%env_asan_abi_opts=',
+ 'env ASAN_ABI_OPTIONS=' + default_asan_abi_opts_str))
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# GCC-ASan doesn't link in all the necessary libraries automatically, so
+# we have to do it ourselves.
+extra_link_flags = []
+
+# Setup default compiler flags used with -fsanitize=address option.
+# FIXME: Review the set of required flags and check if it can be reduced.
+target_cflags = [get_required_attr(config, "target_cflags")] + extra_link_flags
+target_cxxflags = config.cxx_mode_flags + target_cflags
+clang_asan_abi_static_cflags = (["-fsanitize=address",
+"-fsanitize-stable-abi",
+"-mno-omit-leaf-frame-pointer",
+"-fno-omit-frame-pointer",
+"-fno-optimize-sibling-calls"] +
+config.debug_info_flags + target_cflags)
+clang_asan_abi_static_cxxflags = config.cxx_mode_flags + clang_asan_abi_static_cflags
+
+config.available_features.add("asan_abi-static-runtime")
+clang_asan_abi_cflags = clang_asan_abi_static_cflags
+clang_asan_abi_cxxflags = clang_asan_abi_static_cxxflags
+
+def build_invocation(compile_flags):
+  return " " + " ".join([config.clang] + compile_flags) + " "
+
+config.substitutions.append( ("%clang ", build_invocation(target_cflags)) )
+config.substitutions.append( ("%clangxx ", build_invocation(target_cxxflags)) )
+config.substitutions.append( ("%clang_asan_abi ", build_invocation(clang_asan_abi_cflags)) )
+config.substitutions.append( ("%clangxx_asan_abi ", build_invocation(clang_asan_abi_cxxflags)) )
+
+libasan_abi_path = os.path.join(config.compiler_rt_libdir, 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 521479.
rsundahl added a comment.

Added testing. Removed asan_abi_shim.h.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/asan_abi.md
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc
  compiler-rt/test/asan_abi/CMakeLists.txt
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
  compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp
  compiler-rt/test/asan_abi/lit.cfg.py
  compiler-rt/test/asan_abi/lit.site.cfg.py.in

Index: compiler-rt/test/asan_abi/lit.site.cfg.py.in
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.site.cfg.py.in
@@ -0,0 +1,18 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.name_suffix = "@ASAN_ABI_TEST_CONFIG_SUFFIX@"
+config.target_cflags = "@ASAN_ABI_TEST_TARGET_CFLAGS@"
+config.clang = "@ASAN_ABI_TEST_TARGET_CC@"
+config.bits = "@ASAN_ABI_TEST_BITS@"
+config.arm_thumb = "@COMPILER_RT_ARM_THUMB@"
+config.apple_platform = "@ASAN_ABI_TEST_APPLE_PLATFORM@"
+config.apple_platform_min_deployment_target_flag = "@ASAN_ABI_TEST_MIN_DEPLOYMENT_TARGET_FLAG@"
+config.asan_abi_dynamic = @ASAN_ABI_TEST_DYNAMIC@
+config.target_arch = "@ASAN_ABI_TEST_TARGET_ARCH@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@ASAN_ABI_LIT_SOURCE_DIR@/lit.cfg.py")
Index: compiler-rt/test/asan_abi/lit.cfg.py
===
--- /dev/null
+++ compiler-rt/test/asan_abi/lit.cfg.py
@@ -0,0 +1,84 @@
+# -*- Python -*-
+
+import os
+import platform
+import re
+
+import lit.formats
+
+# Get shlex.quote if available (added in 3.3), and fall back to pipes.quote if
+# it's not available.
+try:
+  import shlex
+  sh_quote = shlex.quote
+except:
+  import pipes
+  sh_quote = pipes.quote
+
+def get_required_attr(config, attr_name):
+  attr_value = getattr(config, attr_name, None)
+  if attr_value == None:
+lit_config.fatal(
+  "No attribute %r in test configuration! You may need to run "
+  "tests from your build directory or add this attribute "
+  "to lit.site.cfg.py " % attr_name)
+  return attr_value
+
+# Setup config name.
+config.name = 'AddressSanitizerABI' + config.name_suffix
+
+# Platform-specific default ASAN_ABI_OPTIONS for lit tests.
+default_asan_abi_opts = list(config.default_sanitizer_opts)
+
+default_asan_abi_opts_str = ':'.join(default_asan_abi_opts)
+if default_asan_abi_opts_str:
+  config.environment['ASAN_ABI_OPTIONS'] = default_asan_abi_opts_str
+  default_asan_abi_opts_str += ':'
+config.substitutions.append(('%env_asan_abi_opts=',
+ 'env ASAN_ABI_OPTIONS=' + default_asan_abi_opts_str))
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# GCC-ASan doesn't link in all the necessary libraries automatically, so
+# we have to do it ourselves.
+extra_link_flags = []
+
+# Setup default compiler flags used with -fsanitize=address option.
+# FIXME: Review the set of required flags and check if it can be reduced.
+target_cflags = [get_required_attr(config, "target_cflags")] + extra_link_flags
+target_cxxflags = config.cxx_mode_flags + target_cflags
+clang_asan_abi_static_cflags = (["-fsanitize=address",
+"-fsanitize-stable-abi",
+"-mno-omit-leaf-frame-pointer",
+"-fno-omit-frame-pointer",
+"-fno-optimize-sibling-calls"] +
+config.debug_info_flags + target_cflags)
+clang_asan_abi_static_cxxflags = config.cxx_mode_flags + clang_asan_abi_static_cflags
+
+config.available_features.add("asan_abi-static-runtime")
+clang_asan_abi_cflags = clang_asan_abi_static_cflags
+clang_asan_abi_cxxflags = clang_asan_abi_static_cxxflags
+
+def build_invocation(compile_flags):
+  return " " + " ".join([config.clang] + compile_flags) + " "
+
+config.substitutions.append( ("%clang ", build_invocation(target_cflags)) )
+config.substitutions.append( ("%clangxx ", build_invocation(target_cxxflags)) )
+config.substitutions.append( ("%clang_asan_abi ", build_invocation(clang_asan_abi_cflags)) )
+config.substitutions.append( ("%clangxx_asan_abi ", build_invocation(clang_asan_abi_cxxflags)) )
+
+libasan_abi_path = os.path.join(config.compiler_rt_libdir, 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

In D143675#4330599 , @thetruestblue 
wrote:

> Small insignificant note from me: When this lands, please be sure to add me 
> as co-author.
> https://github.blog/2018-01-29-commit-together-with-co-authors/

I've not seen this before @thetruestblue! I certainly will do so!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

In D143675#4310903 , @eugenis wrote:

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

Right, we should be using it... We will add a test that compiles and links to 
it as affirmation that we have covered all of the entrypoints that ASan 
generates. It's also the case that asan_abi_shim.h is redundant since 
asan_abi_shim.cpp now gets its declarations from 
../asan/asan_interface_internal.h which is of course the source of truth for 
what the shim should be implementing. We will remove that as well and update. 
Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Brittany Blue Gaston via Phabricator via cfe-commits]

thetruestblue added a comment.

Small insignificant note from me: When this lands, please be sure to add me as 
co-author.
https://github.blog/2018-01-29-commit-together-with-co-authors/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka accepted this revision.
vitalybuka added 2 blocking reviewer(s): eugenis, MaskRay.
vitalybuka added a comment.

In D143675#4310734 , @rsundahl wrote:

> @kcc @eugenis @MaskRay @vitalybuka Ok to go with this? All new functionality 
> is under the added flag so not expecting any surprises. Rename 
> asabi->asan_abi as suggested.

Thanks, asan_abi LGTM.

I don't have good reasons to object that patch, but I suspect it's sub-optimal. 
But we may get a valuable expirience.

> Rather than adding a lot of conditional code to the LLVM instrumentation phase

We do this for hwasan for Android, and to some extent msan for Chromium. 
@eugenis maybe can share more info.

> Based on previous discussions about this topic, our understanding is that 
> freezing the present ABI would impose an excessive burden on other sanitizer 
> developers and for unrelated platforms.

I guess we just have no way to enforce that. A couple of buildbots with "stable 
clang" + "HEAD runtime" and "HEAD clang" + "stable runtime" which do some 
non-tivial build, e.g. clang bootstrap can enforce that. We can at list to 
enforce default set of flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl marked an inline comment as done.
rsundahl added a comment.

@kcc @eugenis @MaskRay @vitalybuka Ok to go with this? All new functionality is 
under the added flag so not expecting any surprises. Rename asabi->asan_abi as 
suggested.




Comment at: compiler-rt/lib/asabi/CMakeLists.txt:2
+# Build for the ASAN Stable ABI runtime support library.
+set(ASABI_SOURCES
+  asabi_shim.cpp

rsundahl wrote:
> vitalybuka wrote:
> > does it need to be asabi?
> > maybe better asan_abi, files and macro?
> The idea is that "asabi" replaces "asan" (where the "s" stands in for 
> "stable"), but I understand the distraction of sounding like a hot condiment! 
> I wonder what you think of "asan_stable" (over "asan_stable_abi" or 
> "asan_abi" as you suggest). I am more at ease with emphasizing the "stable" 
> over the "abi" since both "asan" and "asan_stable" share ABI-ness without 
> calling it out. This might read and organize a bit more naturally with what 
> we already have. Thanks for the input.
Changed "asabi" namespace to "asan_abi". Applied to files, directories and 
contents.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 518552.
rsundahl added a comment.

Rename asabi->asan_abi


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/asan_abi.md
  compiler-rt/lib/asan_abi/CMakeLists.txt
  compiler-rt/lib/asan_abi/asan_abi.cpp
  compiler-rt/lib/asan_abi/asan_abi.h
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/lib/asan_abi/asan_abi_shim.h
  compiler-rt/test/asan_abi/CMakeLists.txt

Index: compiler-rt/test/asan_abi/CMakeLists.txt
===
--- /dev/null
+++ compiler-rt/test/asan_abi/CMakeLists.txt
@@ -0,0 +1 @@
+#TODO: Setup asan_abi test suite.
Index: compiler-rt/lib/asan_abi/asan_abi_shim.h
===
--- /dev/null
+++ compiler-rt/lib/asan_abi/asan_abi_shim.h
@@ -0,0 +1,224 @@
+//===-asan_abi_shim.h - ASan Stable ABI Shim Interfac--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef ASAN_ABI_SHIM_H
+#define ASAN_ABI_SHIM_H
+
+typedef unsigned long uptr;
+typedef unsigned long u64;
+typedef unsigned int u32;
+
+extern void __asan_register_image_globals(uptr *flag);
+extern void __asan_unregister_image_globals(uptr *flag);
+
+extern void __asan_register_elf_globals(uptr *flag, void *start, void *stop);
+extern void __asan_unregister_elf_globals(uptr *flag, void *start, void *stop);
+
+extern void __asan_register_globals(__asan_global *globals, uptr n);
+extern void __asan_unregister_globals(__asan_global *globals, uptr n);
+
+extern void __asan_before_dynamic_init(const char *module_name);
+extern void __asan_after_dynamic_init(void);
+
+// Interceptors
+extern void *__asan_memcpy(void *dst, const void *src, uptr size);
+extern void *__asan_memset(void *s, int c, uptr n);
+extern void *__asan_memmove(void *dest, const void *src, uptr n);
+
+// RTL (Interface)
+extern void __asan_init(void);
+extern void __asan_version_mismatch_check(void);
+extern void __asan_handle_no_return(void);
+
+// RTL (Variables)
+extern uptr __asan_shadow_memory_dynamic_address;
+extern int __asan_option_detect_stack_use_after_return;
+
+// RTL (Report)
+extern void __asan_report_load1(uptr addr);
+extern void __asan_report_load2(uptr addr);
+extern void __asan_report_load4(uptr addr);
+extern void __asan_report_load8(uptr addr);
+extern void __asan_report_load16(uptr addr);
+extern void __asan_report_load_n(uptr addr, uptr size);
+extern void __asan_report_store1(uptr addr);
+extern void __asan_report_store2(uptr addr);
+extern void __asan_report_store4(uptr addr);
+extern void __asan_report_store8(uptr addr);
+extern void __asan_report_store16(uptr addr);
+extern void __asan_report_store_n(uptr addr, uptr size);
+
+// RTL (Report-experimental)
+extern void __asan_report_exp_load1(uptr addr, u32 exp);
+extern void __asan_report_exp_load2(uptr addr, u32 exp);
+extern void __asan_report_exp_load4(uptr addr, u32 exp);
+extern void __asan_report_exp_load8(uptr addr, u32 exp);
+extern void __asan_report_exp_load16(uptr addr, u32 exp);
+extern void __asan_report_exp_load_n(uptr addr, uptr size, u32 exp);
+extern void __asan_report_exp_store1(uptr addr, u32 exp);
+extern void __asan_report_exp_store2(uptr addr, u32 exp);
+extern void __asan_report_exp_store4(uptr addr, u32 exp);
+extern void __asan_report_exp_store8(uptr addr, u32 exp);
+extern void __asan_report_exp_store16(uptr addr, u32 exp);
+extern void __asan_report_exp_store_n(uptr addr, uptr size, u32 exp);
+
+// RTL (Report-noabort)
+extern void __asan_report_load1_noabort(uptr addr);
+extern void __asan_report_load2_noabort(uptr addr);
+extern void __asan_report_load4_noabort(uptr addr);
+extern void __asan_report_load8_noabort(uptr addr);
+extern void __asan_report_load16_noabort(uptr addr);
+extern void __asan_report_load_n_noabort(uptr addr, uptr size);
+extern void __asan_report_store1_noabort(uptr addr);
+extern void __asan_report_store2_noabort(uptr addr);
+extern void __asan_report_store4_noabort(uptr addr);
+extern void __asan_report_store8_noabort(uptr addr);
+extern void __asan_report_store16_noabort(uptr addr);
+extern void __asan_report_store_n_noabort(uptr addr, uptr size);
+
+// RTL (Access)
+extern void __asan_load1(uptr p);
+extern void __asan_load2(uptr p);
+extern void __asan_load4(uptr p);
+extern void __asan_load8(uptr p);
+extern void __asan_load16(uptr p);
+extern void __asan_loadN(uptr p, uptr size);
+extern void __asan_store1(uptr p);

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added inline comments.



Comment at: compiler-rt/lib/asabi/CMakeLists.txt:2
+# Build for the ASAN Stable ABI runtime support library.
+set(ASABI_SOURCES
+  asabi_shim.cpp

vitalybuka wrote:
> does it need to be asabi?
> maybe better asan_abi, files and macro?
The idea is that "asabi" replaces "asan" (where the "s" stands in for 
"stable"), but I understand the distraction of sounding like a hot condiment! I 
wonder what you think of "asan_stable" (over "asan_stable_abi" or "asan_abi" as 
you suggest). I am more at ease with emphasizing the "stable" over the "abi" 
since both "asan" and "asan_stable" share ABI-ness without calling it out. This 
might read and organize a bit more naturally with what we already have. Thanks 
for the input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Driver/Options.td:1785
HelpText<"Use default code 
inlining logic for the address sanitizer">;
+def fsanitize_address_stable_abi : Flag<["-"], "fsanitize-address-stable-abi">,
+Group,

vitalybuka wrote:
> how likely you will need thus for  other sanitizers in future
> should this be rather -fsanitize-stable-abi which is ignore for now for other 
> sanitizers?
Thanks @vitalybuka. I agree and made this change. For now I still only consume 
the flag if sanitizer=address so that we continue to get the unused option 
warning in the other cases and left the test for that warning intact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 515559.
rsundahl added a comment.

Rename fsanitize_address_stable_abi to fsanitize_stable_abi


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/asabi.md
  compiler-rt/lib/asabi/CMakeLists.txt
  compiler-rt/lib/asabi/asabi.cpp
  compiler-rt/lib/asabi/asabi.h
  compiler-rt/lib/asabi/asabi_shim.cpp
  compiler-rt/lib/asabi/asabi_shim.h
  compiler-rt/test/asabi/CMakeLists.txt

Index: compiler-rt/test/asabi/CMakeLists.txt
===
--- /dev/null
+++ compiler-rt/test/asabi/CMakeLists.txt
@@ -0,0 +1 @@
+#TODO: Setup asabi test suite.
\ No newline at end of file
Index: compiler-rt/lib/asabi/asabi_shim.h
===
--- /dev/null
+++ compiler-rt/lib/asabi/asabi_shim.h
@@ -0,0 +1,224 @@
+//===-asabi_shim.h - ASan Stable ABI Shim Interface===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef ASABI_SHIM_H
+#define ASABI_SHIM_H
+
+typedef unsigned long uptr;
+typedef unsigned long u64;
+typedef unsigned int u32;
+
+extern void __asan_register_image_globals(uptr *flag);
+extern void __asan_unregister_image_globals(uptr *flag);
+
+extern void __asan_register_elf_globals(uptr *flag, void *start, void *stop);
+extern void __asan_unregister_elf_globals(uptr *flag, void *start, void *stop);
+
+extern void __asan_register_globals(__asan_global *globals, uptr n);
+extern void __asan_unregister_globals(__asan_global *globals, uptr n);
+
+extern void __asan_before_dynamic_init(const char *module_name);
+extern void __asan_after_dynamic_init(void);
+
+// Interceptors
+extern void *__asan_memcpy(void *dst, const void *src, uptr size);
+extern void *__asan_memset(void *s, int c, uptr n);
+extern void *__asan_memmove(void *dest, const void *src, uptr n);
+
+// RTL (Interface)
+extern void __asan_init(void);
+extern void __asan_version_mismatch_check(void);
+extern void __asan_handle_no_return(void);
+
+// RTL (Variables)
+extern uptr __asan_shadow_memory_dynamic_address;
+extern int __asan_option_detect_stack_use_after_return;
+
+// RTL (Report)
+extern void __asan_report_load1(uptr addr);
+extern void __asan_report_load2(uptr addr);
+extern void __asan_report_load4(uptr addr);
+extern void __asan_report_load8(uptr addr);
+extern void __asan_report_load16(uptr addr);
+extern void __asan_report_load_n(uptr addr, uptr size);
+extern void __asan_report_store1(uptr addr);
+extern void __asan_report_store2(uptr addr);
+extern void __asan_report_store4(uptr addr);
+extern void __asan_report_store8(uptr addr);
+extern void __asan_report_store16(uptr addr);
+extern void __asan_report_store_n(uptr addr, uptr size);
+
+// RTL (Report-experimental)
+extern void __asan_report_exp_load1(uptr addr, u32 exp);
+extern void __asan_report_exp_load2(uptr addr, u32 exp);
+extern void __asan_report_exp_load4(uptr addr, u32 exp);
+extern void __asan_report_exp_load8(uptr addr, u32 exp);
+extern void __asan_report_exp_load16(uptr addr, u32 exp);
+extern void __asan_report_exp_load_n(uptr addr, uptr size, u32 exp);
+extern void __asan_report_exp_store1(uptr addr, u32 exp);
+extern void __asan_report_exp_store2(uptr addr, u32 exp);
+extern void __asan_report_exp_store4(uptr addr, u32 exp);
+extern void __asan_report_exp_store8(uptr addr, u32 exp);
+extern void __asan_report_exp_store16(uptr addr, u32 exp);
+extern void __asan_report_exp_store_n(uptr addr, uptr size, u32 exp);
+
+// RTL (Report-noabort)
+extern void __asan_report_load1_noabort(uptr addr);
+extern void __asan_report_load2_noabort(uptr addr);
+extern void __asan_report_load4_noabort(uptr addr);
+extern void __asan_report_load8_noabort(uptr addr);
+extern void __asan_report_load16_noabort(uptr addr);
+extern void __asan_report_load_n_noabort(uptr addr, uptr size);
+extern void __asan_report_store1_noabort(uptr addr);
+extern void __asan_report_store2_noabort(uptr addr);
+extern void __asan_report_store4_noabort(uptr addr);
+extern void __asan_report_store8_noabort(uptr addr);
+extern void __asan_report_store16_noabort(uptr addr);
+extern void __asan_report_store_n_noabort(uptr addr, uptr size);
+
+// RTL (Access)
+extern void __asan_load1(uptr p);
+extern void __asan_load2(uptr p);
+extern void __asan_load4(uptr p);
+extern void __asan_load8(uptr p);
+extern void __asan_load16(uptr p);
+extern void __asan_loadN(uptr p, uptr size);
+extern void __asan_store1(uptr p);

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

In D143675#4281673 , @rsundahl wrote:

> @kcc @eugenis @MaskRay @vitalybuka Ok to go with this? All new functionality 
> is under the added flag so not expecting any surprises.

I don't have reasons to block this.




Comment at: clang/include/clang/Driver/Options.td:1785
HelpText<"Use default code 
inlining logic for the address sanitizer">;
+def fsanitize_address_stable_abi : Flag<["-"], "fsanitize-address-stable-abi">,
+Group,

how likely you will need thus for  other sanitizers in future
should this be rather -fsanitize-stable-abi which is ignore for now for other 
sanitizers?



Comment at: compiler-rt/lib/asabi/CMakeLists.txt:2
+# Build for the ASAN Stable ABI runtime support library.
+set(ASABI_SOURCES
+  asabi_shim.cpp

does it need to be asabi?
maybe better asan_abi, files and macro?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

@kcc @eugenis @MaskRay @vitalybuka Ok to go with this? All new functionality is 
under the added flag so not expecting any surprises.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Kuba (Brecka) Mracek via Phabricator via cfe-commits]

kubamracek added a comment.

@kcc Any takes from you on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Brittany Blue Gaston via Phabricator via cfe-commits]

thetruestblue added a comment.

Currently & ideally asabi_shim.h is unnecessary -- but we hope to use only 
headers from ../asan/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 501358.
rsundahl added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/asabi.md
  compiler-rt/lib/asabi/CMakeLists.txt
  compiler-rt/lib/asabi/asabi.cpp
  compiler-rt/lib/asabi/asabi.h
  compiler-rt/lib/asabi/asabi_shim.cpp
  compiler-rt/lib/asabi/asabi_shim.h
  compiler-rt/test/asabi/CMakeLists.txt

Index: compiler-rt/test/asabi/CMakeLists.txt
===
--- /dev/null
+++ compiler-rt/test/asabi/CMakeLists.txt
@@ -0,0 +1 @@
+#TODO: Setup asabi test suite.
\ No newline at end of file
Index: compiler-rt/lib/asabi/asabi_shim.h
===
--- /dev/null
+++ compiler-rt/lib/asabi/asabi_shim.h
@@ -0,0 +1,224 @@
+//===-asabi_shim.h - ASan Stable ABI Shim Interface===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef ASABI_SHIM_H
+#define ASABI_SHIM_H
+
+typedef unsigned long uptr;
+typedef unsigned long u64;
+typedef unsigned int u32;
+
+extern void __asan_register_image_globals(uptr *flag);
+extern void __asan_unregister_image_globals(uptr *flag);
+
+extern void __asan_register_elf_globals(uptr *flag, void *start, void *stop);
+extern void __asan_unregister_elf_globals(uptr *flag, void *start, void *stop);
+
+extern void __asan_register_globals(__asan_global *globals, uptr n);
+extern void __asan_unregister_globals(__asan_global *globals, uptr n);
+
+extern void __asan_before_dynamic_init(const char *module_name);
+extern void __asan_after_dynamic_init(void);
+
+// Interceptors
+extern void *__asan_memcpy(void *dst, const void *src, uptr size);
+extern void *__asan_memset(void *s, int c, uptr n);
+extern void *__asan_memmove(void *dest, const void *src, uptr n);
+
+// RTL (Interface)
+extern void __asan_init(void);
+extern void __asan_version_mismatch_check(void);
+extern void __asan_handle_no_return(void);
+
+// RTL (Variables)
+extern uptr __asan_shadow_memory_dynamic_address;
+extern int __asan_option_detect_stack_use_after_return;
+
+// RTL (Report)
+extern void __asan_report_load1(uptr addr);
+extern void __asan_report_load2(uptr addr);
+extern void __asan_report_load4(uptr addr);
+extern void __asan_report_load8(uptr addr);
+extern void __asan_report_load16(uptr addr);
+extern void __asan_report_load_n(uptr addr, uptr size);
+extern void __asan_report_store1(uptr addr);
+extern void __asan_report_store2(uptr addr);
+extern void __asan_report_store4(uptr addr);
+extern void __asan_report_store8(uptr addr);
+extern void __asan_report_store16(uptr addr);
+extern void __asan_report_store_n(uptr addr, uptr size);
+
+// RTL (Report-experimental)
+extern void __asan_report_exp_load1(uptr addr, u32 exp);
+extern void __asan_report_exp_load2(uptr addr, u32 exp);
+extern void __asan_report_exp_load4(uptr addr, u32 exp);
+extern void __asan_report_exp_load8(uptr addr, u32 exp);
+extern void __asan_report_exp_load16(uptr addr, u32 exp);
+extern void __asan_report_exp_load_n(uptr addr, uptr size, u32 exp);
+extern void __asan_report_exp_store1(uptr addr, u32 exp);
+extern void __asan_report_exp_store2(uptr addr, u32 exp);
+extern void __asan_report_exp_store4(uptr addr, u32 exp);
+extern void __asan_report_exp_store8(uptr addr, u32 exp);
+extern void __asan_report_exp_store16(uptr addr, u32 exp);
+extern void __asan_report_exp_store_n(uptr addr, uptr size, u32 exp);
+
+// RTL (Report-noabort)
+extern void __asan_report_load1_noabort(uptr addr);
+extern void __asan_report_load2_noabort(uptr addr);
+extern void __asan_report_load4_noabort(uptr addr);
+extern void __asan_report_load8_noabort(uptr addr);
+extern void __asan_report_load16_noabort(uptr addr);
+extern void __asan_report_load_n_noabort(uptr addr, uptr size);
+extern void __asan_report_store1_noabort(uptr addr);
+extern void __asan_report_store2_noabort(uptr addr);
+extern void __asan_report_store4_noabort(uptr addr);
+extern void __asan_report_store8_noabort(uptr addr);
+extern void __asan_report_store16_noabort(uptr addr);
+extern void __asan_report_store_n_noabort(uptr addr, uptr size);
+
+// RTL (Access)
+extern void __asan_load1(uptr p);
+extern void __asan_load2(uptr p);
+extern void __asan_load4(uptr p);
+extern void __asan_load8(uptr p);
+extern void __asan_load16(uptr p);
+extern void __asan_loadN(uptr p, uptr size);
+extern void __asan_store1(uptr p);
+extern void __asan_store2(uptr p);
+extern void 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 501357.
rsundahl added a comment.

Deleted extraneous line from compiler-rt/cmake/config-ix.cmake


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/Driver/fsanitize.c
  clang/test/Sema/attr-riscv-rvv-vector-bits.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/asabi.md
  compiler-rt/lib/asabi/CMakeLists.txt
  compiler-rt/lib/asabi/asabi.cpp
  compiler-rt/lib/asabi/asabi.h
  compiler-rt/lib/asabi/asabi_shim.cpp
  compiler-rt/lib/asabi/asabi_shim.h
  compiler-rt/test/asabi/CMakeLists.txt

Index: compiler-rt/test/asabi/CMakeLists.txt
===
--- /dev/null
+++ compiler-rt/test/asabi/CMakeLists.txt
@@ -0,0 +1 @@
+#TODO: Setup asabi test suite.
\ No newline at end of file
Index: compiler-rt/lib/asabi/asabi_shim.h
===
--- /dev/null
+++ compiler-rt/lib/asabi/asabi_shim.h
@@ -0,0 +1,224 @@
+//===-asabi_shim.h - ASan Stable ABI Shim Interface===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef ASABI_SHIM_H
+#define ASABI_SHIM_H
+
+typedef unsigned long uptr;
+typedef unsigned long u64;
+typedef unsigned int u32;
+
+extern void __asan_register_image_globals(uptr *flag);
+extern void __asan_unregister_image_globals(uptr *flag);
+
+extern void __asan_register_elf_globals(uptr *flag, void *start, void *stop);
+extern void __asan_unregister_elf_globals(uptr *flag, void *start, void *stop);
+
+extern void __asan_register_globals(__asan_global *globals, uptr n);
+extern void __asan_unregister_globals(__asan_global *globals, uptr n);
+
+extern void __asan_before_dynamic_init(const char *module_name);
+extern void __asan_after_dynamic_init(void);
+
+// Interceptors
+extern void *__asan_memcpy(void *dst, const void *src, uptr size);
+extern void *__asan_memset(void *s, int c, uptr n);
+extern void *__asan_memmove(void *dest, const void *src, uptr n);
+
+// RTL (Interface)
+extern void __asan_init(void);
+extern void __asan_version_mismatch_check(void);
+extern void __asan_handle_no_return(void);
+
+// RTL (Variables)
+extern uptr __asan_shadow_memory_dynamic_address;
+extern int __asan_option_detect_stack_use_after_return;
+
+// RTL (Report)
+extern void __asan_report_load1(uptr addr);
+extern void __asan_report_load2(uptr addr);
+extern void __asan_report_load4(uptr addr);
+extern void __asan_report_load8(uptr addr);
+extern void __asan_report_load16(uptr addr);
+extern void __asan_report_load_n(uptr addr, uptr size);
+extern void __asan_report_store1(uptr addr);
+extern void __asan_report_store2(uptr addr);
+extern void __asan_report_store4(uptr addr);
+extern void __asan_report_store8(uptr addr);
+extern void __asan_report_store16(uptr addr);
+extern void __asan_report_store_n(uptr addr, uptr size);
+
+// RTL (Report-experimental)
+extern void __asan_report_exp_load1(uptr addr, u32 exp);
+extern void __asan_report_exp_load2(uptr addr, u32 exp);
+extern void __asan_report_exp_load4(uptr addr, u32 exp);
+extern void __asan_report_exp_load8(uptr addr, u32 exp);
+extern void __asan_report_exp_load16(uptr addr, u32 exp);
+extern void __asan_report_exp_load_n(uptr addr, uptr size, u32 exp);
+extern void __asan_report_exp_store1(uptr addr, u32 exp);
+extern void __asan_report_exp_store2(uptr addr, u32 exp);
+extern void __asan_report_exp_store4(uptr addr, u32 exp);
+extern void __asan_report_exp_store8(uptr addr, u32 exp);
+extern void __asan_report_exp_store16(uptr addr, u32 exp);
+extern void __asan_report_exp_store_n(uptr addr, uptr size, u32 exp);
+
+// RTL (Report-noabort)
+extern void __asan_report_load1_noabort(uptr addr);
+extern void __asan_report_load2_noabort(uptr addr);
+extern void __asan_report_load4_noabort(uptr addr);
+extern void __asan_report_load8_noabort(uptr addr);
+extern void __asan_report_load16_noabort(uptr addr);
+extern void __asan_report_load_n_noabort(uptr addr, uptr size);
+extern void __asan_report_store1_noabort(uptr addr);
+extern void __asan_report_store2_noabort(uptr addr);
+extern void __asan_report_store4_noabort(uptr addr);
+extern void __asan_report_store8_noabort(uptr addr);
+extern void __asan_report_store16_noabort(uptr addr);
+extern void __asan_report_store_n_noabort(uptr addr, uptr size);
+
+// RTL (Access)
+extern void __asan_load1(uptr p);
+extern void __asan_load2(uptr p);
+extern void __asan_load4(uptr p);
+extern 

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

In D143675#4160084 , @vitalybuka 
wrote:

> Usually freezing signatures is not a big concern, we can agree to preserve 
> existing functions.
> The stuff like ASanStackFrameLayout is the concern. compiler and runtime must 
> agree on data layout. The same for global.

Yes. Presently, the address sanitizer code generator is very aware of the stack 
frame layout and how it will be poisoned up to and including the values to be 
stored in the shadow memory. I don't think that the runtime actually shares 
ASanStackFrameLayout at the moment but certainly would be supportive if under 
this proposed flag (where everything gets outlined), we actually passed some 
canonicalized/serialized ASanStackFrameLayout object to the runtime where it 
would be free to implement as appropriate. We're not proposing this kind of 
change at this time but may be worth doing in some future effort to separate 
instrumentation from implementation. (Similar arguments apply under this flag 
for globals).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Brittany Blue Gaston via Phabricator via cfe-commits]

thetruestblue added inline comments.



Comment at: compiler-rt/cmake/config-ix.cmake:734
   set(COMPILER_RT_HAS_ASAN TRUE)
+  set(COMPILER_RT_HAS_ASABI TRUE)
 else()

This was an artifact leftover from some of my cmake changes. This line needs to 
be removed.. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

Added to Discourse: 
https://discourse.llvm.org/t/darwin-sanitizers-stable-abi/68834


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

Usually freezing signatures is not a big concern, we can agree to preserve 
existing functions.
The stuff like ASanStackFrameLayout is the concern. compiler and runtime must 
agree on data layout. The same for global.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 501308.
rsundahl added a comment.
Herald added subscribers: cfe-commits, luke, yaneury, supersymetrie, 
Chia-hungDuan, pcwang-thead, frasercrmck, luismarques, apazos, sameer.abuasal, 
s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, 
edward-jones, zzheng, jrtc27, niosHD, cryptoad, sabuasal, simoncook, johnrusso, 
rbar, asb.
Herald added a project: clang.

Moved and renamed files using asabi namespace. Added CMake scaffolding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/Driver/fsanitize.c
  clang/test/Sema/attr-riscv-rvv-vector-bits.c
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/docs/asabi.md
  compiler-rt/lib/asabi/CMakeLists.txt
  compiler-rt/lib/asabi/asabi.cpp
  compiler-rt/lib/asabi/asabi.h
  compiler-rt/lib/asabi/asabi_shim.cpp
  compiler-rt/lib/asabi/asabi_shim.h
  compiler-rt/test/asabi/CMakeLists.txt

Index: compiler-rt/test/asabi/CMakeLists.txt
===
--- /dev/null
+++ compiler-rt/test/asabi/CMakeLists.txt
@@ -0,0 +1 @@
+#TODO: Setup asabi test suite.
\ No newline at end of file
Index: compiler-rt/lib/asabi/asabi_shim.h
===
--- /dev/null
+++ compiler-rt/lib/asabi/asabi_shim.h
@@ -0,0 +1,224 @@
+//===-asabi_shim.h - ASan Stable ABI Shim Interface===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef ASABI_SHIM_H
+#define ASABI_SHIM_H
+
+typedef unsigned long uptr;
+typedef unsigned long u64;
+typedef unsigned int u32;
+
+extern void __asan_register_image_globals(uptr *flag);
+extern void __asan_unregister_image_globals(uptr *flag);
+
+extern void __asan_register_elf_globals(uptr *flag, void *start, void *stop);
+extern void __asan_unregister_elf_globals(uptr *flag, void *start, void *stop);
+
+extern void __asan_register_globals(__asan_global *globals, uptr n);
+extern void __asan_unregister_globals(__asan_global *globals, uptr n);
+
+extern void __asan_before_dynamic_init(const char *module_name);
+extern void __asan_after_dynamic_init(void);
+
+// Interceptors
+extern void *__asan_memcpy(void *dst, const void *src, uptr size);
+extern void *__asan_memset(void *s, int c, uptr n);
+extern void *__asan_memmove(void *dest, const void *src, uptr n);
+
+// RTL (Interface)
+extern void __asan_init(void);
+extern void __asan_version_mismatch_check(void);
+extern void __asan_handle_no_return(void);
+
+// RTL (Variables)
+extern uptr __asan_shadow_memory_dynamic_address;
+extern int __asan_option_detect_stack_use_after_return;
+
+// RTL (Report)
+extern void __asan_report_load1(uptr addr);
+extern void __asan_report_load2(uptr addr);
+extern void __asan_report_load4(uptr addr);
+extern void __asan_report_load8(uptr addr);
+extern void __asan_report_load16(uptr addr);
+extern void __asan_report_load_n(uptr addr, uptr size);
+extern void __asan_report_store1(uptr addr);
+extern void __asan_report_store2(uptr addr);
+extern void __asan_report_store4(uptr addr);
+extern void __asan_report_store8(uptr addr);
+extern void __asan_report_store16(uptr addr);
+extern void __asan_report_store_n(uptr addr, uptr size);
+
+// RTL (Report-experimental)
+extern void __asan_report_exp_load1(uptr addr, u32 exp);
+extern void __asan_report_exp_load2(uptr addr, u32 exp);
+extern void __asan_report_exp_load4(uptr addr, u32 exp);
+extern void __asan_report_exp_load8(uptr addr, u32 exp);
+extern void __asan_report_exp_load16(uptr addr, u32 exp);
+extern void __asan_report_exp_load_n(uptr addr, uptr size, u32 exp);
+extern void __asan_report_exp_store1(uptr addr, u32 exp);
+extern void __asan_report_exp_store2(uptr addr, u32 exp);
+extern void __asan_report_exp_store4(uptr addr, u32 exp);
+extern void __asan_report_exp_store8(uptr addr, u32 exp);
+extern void __asan_report_exp_store16(uptr addr, u32 exp);
+extern void __asan_report_exp_store_n(uptr addr, uptr size, u32 exp);
+
+// RTL (Report-noabort)
+extern void __asan_report_load1_noabort(uptr addr);
+extern void __asan_report_load2_noabort(uptr addr);
+extern void __asan_report_load4_noabort(uptr addr);
+extern void __asan_report_load8_noabort(uptr addr);
+extern void __asan_report_load16_noabort(uptr addr);
+extern void __asan_report_load_n_noabort(uptr addr, uptr size);
+extern void __asan_report_store1_noabort(uptr addr);
+extern void __asan_report_store2_noabort(uptr addr);