[PATCH] D20084: [sanitizer] Initial implementation of a Hardened Allocator

[Filipe Cabecinhas via llvm-commits]

filcab added inline comments.

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:278
@@ +277,3 @@
+  void Initialize(const AllocatorOptions &options) {
+    CHECK(TestCPUFeature(SSE4_2)); // for crc32
+    DeallocationTypeMismatch = options.DeallocationTypeMismatch;
----------------
glider wrote:
> filcab wrote:
> > glider wrote:
> > > Despite SSE 4.2 may be quite common at Google, I don't think it's a good idea to bail out if it's unsupported.
> > > Note TestCPUFeature() doesn't work on AMD processors yet.
> > Source files are compiled assuming that feature is available.
> > We'll have to add a fallback checksum (plus change build and this check) to address this comment.
> > 
> > I would be ok with keeping the SSE4.2 requirement until we get a non-zero amount of requests/bug reports.
> > 
> > P.S: http://store.steampowered.com/hwsurvey (first result for "hardware survey". It's clearly biased, but I'd guess developer CPUs are also biased to be more recent/powerful than an average computer) puts SSE4.2 adoption at ~80%.
> > 
> Well, IIUC right now the implementation just aborts for AMD processors, which are among those ~80%.
I was just talking about the SSE4.2 part. Sorry, I was going to comment on the CPUID thing but forgot. Doing it now.

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:64
@@ +63,3 @@
+    case SSE4_2:
+      return ((kCPUInfo.ecx >> 20) & 0x1) != 0;
+    case RDRAND:
----------------
Same on AMD (source: https://support.amd.com/TechDocs/25481.pdf):
"
CPUID Fn0000_0001_ECX Feature Identifiers
...
20 SSE42: SSE4.2 instruction support.
"

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:66
@@ +65,3 @@
+    case RDRAND:
+      return ((kCPUInfo.ecx >> 30) & 0x1) != 0;
+    default:
----------------
Doesn't exist on AMD.
Since SSE4.2 is required for this, you'll need to implement SSE4.2 detection for other CPU brands.
RDRAND seems to exist only on Intel CPUs and there's a fallback path, so having it only on Intel doesn't seem like a problem.


http://reviews.llvm.org/D20084