[PATCH] D12707: [PATCH] [sanitizers] Add MSan support for AArch64

Fri Sep 11 05:33:46 PDT 2015

zatrazz added inline comments.

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:125
@@ +124,3 @@
+// AArch64 has 3 VMA sizes: 39, 42 and 48.
+#ifndef SANITIZER_AARCH64_VMA
+# define SANITIZER_AARCH64_VMA 39
----------------
rengolin wrote:
> kristof.beyls wrote:
> > rengolin wrote:
> > > eugenis wrote:
> > > > kristof.beyls wrote:
> > > > > eugenis wrote:
> > > > > > zatrazz wrote:
> > > > > > > rengolin wrote:
> > > > > > > > samsonov wrote:
> > > > > > > > > eugenis wrote:
> > > > > > > > > > What's that?
> > > > > > > > > > It's not even attached to any cmake setting.
> > > > > > > > > > Does it mean that a typical clang binary can target all of x86/arm/ppc/etc, 32/64, but only one fixed aarch64 kernel configuration?
> > > > > > > > > > 
> > > > > > > > > > I really think this should be a compiler flag.
> > > > > > > > > > 
> > > > > > > > > > And it should be possible to remove the corresponding macro in compiler-rt as well by exporting this setting through a global variable in the compiled objects (see __msan_keep_going or the ASAN_FLEXIBLE_MAPPING thing).
> > > > > > > > > > 
> > > > > > > > > +1.
> > > > > > > > > 
> > > > > > > > > I'm sorry that we didn't figure it out promptly during reviews where this define was added to another sanitizers. We should rework this - building compiler and runtimes with a secret define is not the way to go.
> > > > > > > > > What's that?
> > > > > > > > 
> > > > > > > > An unfortunate choice. We're in need of a better one.
> > > > > > > > 
> > > > > > > > > It's not even attached to any cmake setting.
> > > > > > > > 
> > > > > > > > CFLAGS. I know...
> > > > > > > > 
> > > > > > > > > Does it mean that a typical clang binary can target all of x86/arm/ppc/etc, 32/64, but only one fixed aarch64 kernel configuration?
> > > > > > > > 
> > > > > > > > I think so. If we want to target both 39 and 42, we'd have to create two separate libraries every time. With 48 coming to be the default in a year or so, we'd have to keep three. If that's acceptable, than I think we should create them all every time. If not, we need a way to select them at build time.
> > > > > > > > 
> > > > > > > > > I really think this should be a compiler flag.
> > > > > > > > 
> > > > > > > > Can you expand on that? This is a temporary implementation detail that is very specific to AArch64, and one that will disappear soon.
> > > > > > > > 
> > > > > > > > Currently, it *is* a compiler flag. Not a pretty one, but one that we expect to be used *very* rarely. In the future, when it all becomes 48 bits, it'll be used even less, and possibly deprecated altogether when we give up supporting the "old" VMA configurations.
> > > > > > > > 
> > > > > > > > Creating an -mvma or anything like that would be kinda pointless, and harder to revert later.
> > > > > > > > What's that?
> > > > > > > > It's not even attached to any cmake setting.
> > > > > > > > Does it mean that a typical clang binary can target all of x86/arm/ppc/etc, 32/64, but only one fixed aarch64 kernel configuration?
> > > > > > > > 
> > > > > > > > I really think this should be a compiler flag.
> > > > > > > > 
> > > > > > > > And it should be possible to remove the corresponding macro in compiler-rt as well by exporting this setting through a global variable in the compiled objects (see __msan_keep_going or the ASAN_FLEXIBLE_MAPPING thing).
> > > > > > > 
> > > > > > > I though about adding it as compiler flag and let the compiler/user decide to which VMA to use. However, as pointed out by rengolin it would require to build multiple sanitizer libraries for each VMA and select them at link time. If it is the desired target I can evaluate this option, however I would like to avoid such complication from the fact recent kernel change default VMA to 48 bits and the idea is to use only this config.
> > > > > > > 
> > > > > > It should be possible to have a single runtime library that would detect VMA at runtime. Maybe it would cost us a percent or two of performance.
> > > > > > 
> > > > > > Having said that, I don't think this macro, as it is, would cause us any problems in practice, and I don't mind if you go ahead with this patch.
> > > > > > 
> > > > > > Two questions:
> > > > > > 1. Does lower VMA setting work on kernels configured for higher VMA, or does it have to be an exact match?
> > > > > > 2. is there a runtime check for an incompatible or suboptimal VMA setting?
> > > > > > 
> > > > > I'm a bit confused.
> > > > > It seems to me that this is adding functionality to the code-generation part of the sanitizer, i.e. embedded in the compiler right? And we're making a decision at compiler-construction time about only supporting one variant of AArch64 linux kernels? That doesn't seem right to me as at compiler-construction time you may want to create a compiler that can target all the different variants of linux kernels.
> > > > Yeah, it does sound quite bad.
> > > > I'd also like to argue that lower VMA settings are unlikely to go away in the near future. Even when the linux kernel defaults to 48, there are platforms using older kernels, already released devices with at least a few years of lifetime. For example, a toolchain in the Android NDK would need to support VMA variants for a wide set of devices, and bundling a whole extra toolchain or two just for aarch64 is not an option.
> > > > 
> > > > I take back my earlier comment. I don't see a way around -mvma.
> > > > 
> > > > I agree that a runtime VMA detection (i.e. a single runtime library for all VMA variants) is not realistic. Sounds like we'll need to build 3 libraries and pick the correct one in the driver based on -mvma value.
> > > > 
> > > I didn't mean 39/42 are going away, just that we'll probably move the default some time in the future.
> > > 
> > > In the long run, I agree that -mvma will be required, if anything, to pick one of the versions. We could also have the detection in the compiler itself, to act as a default, but due to cross-compilation issues, we'll need the flag anyway.
> > > 
> > > Kristof also mentioned about name mangling as a way to do some run time selection, but that would also require some magic in the run time, either a dispatch table, or self-modifying code at start up (like IFUNC). I don't think we're at the stage that we can take any of these decisions lightly.
> > > 
> > > Given the status of the sanitizer code, where a lot of it is either hard-coded or based on pre-processor macros, trying anything dynamic will be a big refactoring change. One that we may need, granted, but not at the expense of having the sanitizers working on the two VMA configurations we have today in some way.
> > > 
> > > As I said earlier, we're discussing this, and maybe we should put forward a BOF session in the US LLVM to discuss the refactoring, but meanwhile, this allows us to test it on both VMAs as we're doing for the other sanitizers. 
> > > 
> > > We haven't taken any decision, nor we're not changing anything here, just adding functionality.
> > I don't think that name mangling requires dispatch tables or self-modifying code.
> > 
> > I agree that name mangling or another mechanism to fail at binary build/link time rather than at run time can be done as part of a separate patch.
> > 
> > However, I do think that hard-coding/restricting which linux distribution you can target when building the compiler is evil, and this patch shouldn't do that. I therefore think that some command line option indeed is needed with a reasonable default before this patch can land.
> There is a compilation option to control that, -DSANITIZER_AARCH64_VMA=42. I agree it's not the best, but being temporary until we can refactor all the *other* hard-coded stuff to make this work at run time without performance impacts, I think it's acceptable.
This patch itself is a *enablement* one meant to make it possible to run and check msan output in current AArch64 scenarios. The idea is to be able to continuous check for regression by buildbots and add an initial support.

We do agree that current approach (using a compiler flag) is not the best option and it should go away with a better strategy. However, as pointed out I also do agree it should be done in another patch and incrementally. It requires some refactor from compiler-rt to build/generate multiple versions of the required symbols used by the sanitizers and the implementation of the flag mechanism.

My idea is to after finish the msan sanitizer (aarch64-linux now supports asan, tsan, and dfsan) is to work on a mechanism to select the vma at build time and make the compiler-rt transparent to it (either by using name mangling, runtime detection, or multiples compiler-rt builds).

So should we still block this patch based on the assumption that this mechanism will be one used for general deployment? 

http://reviews.llvm.org/D12707