[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 ht

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 (o

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/Optio

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 buil

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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.

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 ACTIO

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 ha

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/D143

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/D143

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 Chang

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 patte

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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" nee

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/O

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 befo

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 compil

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/ http

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 __

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/CMa

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/inclu

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 macr

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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_sta

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/D1436

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 ___

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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/inclu

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 _

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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 SIN

[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

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,