[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D143675#4310904 <https://reviews.llvm.org/D143675#4310904>, @vitalybuka wrote:

> In D143675#4310734 <https://reviews.llvm.org/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