On Tue, 10 Jan 2023 06:57:01 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> This doesn't look "too terrible", but I can't comment on the actual >> poisoning strategies. >> >> Cheers. > >> Forcing 2 reviewers to ensure @dholmes-ora can chime in before moving >> forward. > > Well I won't be able to Review as not familiar enough with the code, so > you'll need a second reviewer anyway. I don't hate this to the point of > outright rejecting it but I do have general concerns about whether we should > be directly supporting such tools in our codebase, and if we should whether > these are the right tools. So I've asked other hotspot folk to chime in. > > > Forcing 2 reviewers to ensure @dholmes-ora can chime in before moving > > > forward. > > > > > > Well I won't be able to Review as not familiar enough with the code, so > > you'll need a second reviewer anyway. I don't hate this to the point of > > outright rejecting it but I do have general concerns about whether we > > should be directly supporting such tools in our codebase, and if we should > > whether these are the right tools. So I've asked other hotspot folk to > > chime in. > > I dislike this too. I wondered whether we could hide it behind an "interface > for asan-like tools", where we have a `os::poison_memory(range)` and > `os::unpoison_memory(range)` function. Those functions could in turn call > whatever tool is configured. At least then we don't pollute the code base > with tool specifics. > > Granted, it sounds a bit fig leafy as long as there is only Asan. But if we > wanted, we could implement a primitive poisoner tool in hotspot by > mprotecting if the range spans whole pages. ASan is really one of a kind in this respect. I am not aware of any other tools like it, due to it requiring compiler support. GCC, Clang, and now [MSVC](https://learn.microsoft.com/en-us/cpp/sanitizers/asan-building?view=msvc-170) all support ASan. Given its wide adoption and touching all major platforms, it's a bit of a de facto standard at this point. It's also noted on MSVC [docs](https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=msvc-170) that additional sanitizers may be supported in the future as well. Also since we are using macros anyway (i.e. `ASAN_POISON_MEMORY_REGION`), it would be fairly easy to swap out the implementation to be something else if wanted such as the suggested primitive poisoner. ------------- PR: https://git.openjdk.org/jdk/pull/11702
