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

[Evgeniy Stepanov via llvm-commits]

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
----------------
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?



http://reviews.llvm.org/D12707