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

Evgeniy Stepanov via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 10 11:52:27 PDT 2015


eugenis 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
----------------
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.



http://reviews.llvm.org/D12707





More information about the llvm-commits mailing list