I have the feeling that I've picked the most awkward of each binary choice here, sorry…
Matthew Malcomson <matthew.malcom...@arm.com> writes: > - Parametrising the tag size is pretty easy towards less bits, the current > code > handles this quite nicely using HWASAN_TAG_SIZE and in fact I've already done > that for a WIP branch for MTE stack tagging. > > Parametrising the tag size to more bits is much more involved -- especially > since the hwasan library uses a u8 data type to represent these tags > everywhere. Hence I figure I'll leave this as it is. > > Does that sound ok? If it's easy to parameterise towards fewer bits, I think it'd be worth having a target twiddle for it and simply asserting that the value is not more than 8. > --- w.r.t. Patch 5 > > - Around hwasan_increment_tag, yes -- the STATIC_ASSERT made the modulus > reduntant, it should have asserted the below (since the "less than or equal" > check still works for the smaller tag size used in MTE rather than fixing it > to > tag_offset). > HWASAN_TAG_SIZE <= sizeof (tag_offset) * CHAR_BIT OK. > - Exporting hwasan_base_ptr (which will be renamed to hwasan_frame_base_ptr). > I currently export this through a function hwasan_frame_base rather than via > an > exported variable. I want to do this so that any use of the base pointer will > be recorded so we know to emit the initialisation (whereas a use of > virtual_stack_vars_rtx can rest assured that the pointer will always be > initialised). The reason for that comment was that the cfgexpand.c code seemed to have to work around the fact that it couldn't directly tell whether a given rtx was hwasan_base_ptr. I think we should have the ability to test that, even if it's via a function rather than by directly exposing the variable. E.g. maybe you could have a little class around it or something, so that the comparison still happens inline via a public member function, but so that it's not possible for cfgexpand.c to use the private value directly. > N.b. This is also related to API for TARGET_MEMTAG_GENTAG (renamed in the > new patch to TARGET_MEMTAG_INSERT_RANDOM_TAG). I have tried to separate > generating a register to be used as hwasan_frame_base_ptr and emitting the RTL > to initialise that register. > That way, code that knows it does not want to emit any RTL can still access > this > variable, knowing that the initialisation will be emitted later. > This is largely because I didn't want to start spreading the possibility of > emitting RTL earlier in the expand pass. At the moment hwasan_emit_prologue > is often the first time that RTL is emitted (unless there are large aligned > variables in the stack -- these are indexed off of an alternate "base > register" > generated using get_dynamic_stack_base, and that function emits some code to > generate that new register). > > However, an alternative API which returns a fresh register would match the > interface of get_dynamic_stack_base which is an existing API in the codebase. Yeah, I think that would be better. > - default_memtag_addtag (now *_add_tag) > I was mentioning compile-time UB, I thought that `plus_constant` taking a > poly_int64 rather than a poly_uint64 meant I can't pass such a large number. No, it's just that C++ forces us to choose between signed or unsigned, even though for modes of N<=64 bits, the offset is really just a signless bag of N bits. I think the reason a signed type was chosen is that, when operating on 32-bit modes, adding int64_t (-1) looks more obvious than adding ~uint64_t (0), since the latter seems to be adding to bits beyond the MSB. > - I have added a comment about C++ exceptions, but thought I'd include a bit > more information about the state of things here. > LLVM have support for C++ exceptions by using a different unwinding > personality > function for all HWASAN tagged frames. That personality function is defined > in > libhwasan, and untags the entire stack frame as we unwind it. > > Unfortunately, that personality function relies on the frame pointer pointing > to > just before the variables on the stack (commented as not being enforced by the > ABI but being a requirement for HWASAN). That holds in LLVM but does not for > GCC. > https://github.com/llvm-mirror/compiler-rt/blob/master/lib/hwasan/ > hwasan_exceptions.cpp#L51 > > I have a hack that modifies that function to use _Unwind_Backtrace to find the > extent of the current frame, and then adds the exception support to GCC based > on > this new personality function. > Since the focus of the implementation in GCC is for the kernel (which doesn't > have C++ exceptions) I don't have plans to turn that into something > release-quality and upstream it. The userspace story for hwasan on anything > other than Android has much bigger difficulties around not using the "platform > ABI", so I don't think putting much effort into C++ exceptions makes sense > right > now. OK. > - About maybe combining the hwasan pass and the asan pass. > Yes we could just branch the "asan" pass based on hwasan vs asan flags. > I would prefer to have the "hwasan" pass separate (I liked having the > "correct" names for the -fdump-tree* arguments and dump files). > That said -- if you feel strongly about this I'll happily change it. Well, I'd argue that “asan” is a correct name too. :-) It feels similar to not having different passes for using (say) glibc vector libm routines and proprietary vector libm routines. The two libm libraries work differently internally and might provide different sets of vector routines, but at the pass level, they achieve the same basic thing and plug into the compiler at the same points. And at a high level, they're both vector libm routines. Here the (old) asan and hwasan are two different, mutually-incompatible ways of providing an address sanitisation feature. Although the details differ, where and how they plug into the compiler is very similar, and the patch is (rightly) reusing some of the asan ifns and enums. Thanks, Richard
