Hi Richard, I've done most of the updates you suggested, but have a few questions to ensure I have the right end of the stick before making the remaining changes.
There are also a few clarifications I'd like to make where I hadn't explained the rationale for certain bits of the original code, and I think that those clarifications may lead to further changes you'd like. I'm putting all the questions and extra clarifications in one email rather than replying to each of the emails in turn. --- w.r.t Patch 1 - You suggested this should also update README.gcc and merge.sh. I believe Martin Liska wanted to perform the merge of libhwasan into the libsanitizer directory, and that seems to make a lot of sense given that he knows a lot more here. He's posted a patch that would do this work here https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556365.html --- w.r.t. Patch 2 - You correctly read that the `configure.tgt` indicates that HWASAN is supported for ilp32, that's my mistake. HWASAN does not actually handle ilp32 (the hwasan library functions `__hwasan_load*` take pointers as arguments and check the top bytes which obviously can't pass that top byte). N.b. I've may have mentioned ilp32 a few times and put comments discussing ptr_mode and Pmode being different, but that's more to do with trying to ensure the code is done "properly" rather than actually ensuring the functionality works on ilp32. --- w.r.t. Patch 4 - 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? --- 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 - 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). 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. - 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. --- w.r.t. Patch 6 - 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. - LLVM does nothing special with strlen, it just doesn't instrument the call. - 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. ________________________________ From: Richard Sandiford <richard.sandif...@arm.com> Sent: 13 October 2020 16:57 To: Matthew Malcomson <matthew.malcom...@arm.com> Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; k...@google.com <k...@google.com>; Richard Earnshaw <richard.earns...@arm.com>; ja...@redhat.com <ja...@redhat.com>; jos...@codesourcery.com <jos...@codesourcery.com>; dvyu...@google.com <dvyu...@google.com>; Kyrylo Tkachov <kyrylo.tkac...@arm.com>; do...@redhat.com <do...@redhat.com>; Martin Liska <mli...@suse.cz> Subject: Re: [PATCH 1/X] libsanitizer: Tie the hwasan library into our build system Sorry for the slow review. Matthew Malcomson <matthew.malcom...@arm.com> writes: > This patch tries to tie libhwasan into the GCC build system in the same way > that the other sanitizer runtime libraries are handled. > > libsanitizer/ChangeLog: > > * Makefile.am: Build libhwasan. > * Makefile.in: Build libhwasan. > * asan/Makefile.in: Build libhwasan. > * configure: Build libhwasan. > * configure.ac: Build libhwasan. > * hwasan/Makefile.am: New file. > * hwasan/Makefile.in: New file. > * hwasan/libtool-version: New file. > * interception/Makefile.in: Build libhwasan. > * libbacktrace/Makefile.in: Build libhwasan. > * libsanitizer.spec.in: Build libhwasan. > * lsan/Makefile.in: Build libhwasan. > * sanitizer_common/Makefile.in: Build libhwasan. > * tsan/Makefile.in: Build libhwasan. > * ubsan/Makefile.in: Build libhwasan. I think this should also update README.gcc and merge.sh. Could you try locally merging in a dummy change to the llvm sources with merge.sh, to make sure it works correctly? > new file mode 100644 > index > 0000000000000000000000000000000000000000..aaa39b4536a5c5f54910a951470814bbc8a20946 > --- /dev/null > +++ b/libsanitizer/hwasan/Makefile.am > @@ -0,0 +1,88 @@ > +AM_CPPFLAGS = -I $(top_srcdir)/include -I $(top_srcdir) > + > +# May be used by toolexeclibdir. > +gcc_version := $(shell @get_gcc_base_ver@ $(top_srcdir)/../gcc/BASE-VER) > + > +DEFS = -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS > -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -DCAN_SANITIZE_UB=0 > -DHWASAN_WITH_INTERCEPTORS=1 > +AM_CXXFLAGS = -Wall -W -Wno-unused-parameter -Wwrite-strings -pedantic > -Wno-long-long -fPIC -fno-builtin -fno-exceptions -fno-rtti > -fomit-frame-pointer -funwind-tables -fvisibility=hidden -Wno-variadic-macros > -fno-ipa-icf I realise this is just taken from the other Makefile.ams, but do you know the reason behind -fomit-frame-pointer? I think we should avoid building aarch64 libraries with that flag unless there's a specific reason. Otherwise looks good to me, although I'm definitely not an expert on this stuff. Thanks, Richard
