[PATCH] D127544: Add no_sanitize('hwaddress') (and 'memtag', but that's a no-op).

[Mitch Phillips via Phabricator via cfe-commits]

hctim added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8849
     break;
-  
   // HLSL attributes:
----------------
aaron.ballman wrote:
> Spurious whitespace change?
unfortunately there's no way in my editor to trim trailing whitespace only on changed lines :(, so i end up fixing things like this drive-by.

let me know if you feel very strongly about this diff and I can kill it, but I personally think the drive-by-fix isn't a huge problem and the alternative of whitespace-fix-only commit seems a bit overkill


================
Comment at: clang/test/CodeGen/hwasan-globals.cpp:1-2
+// RUN: echo "int extra_global;" > %t.extra-source.cpp
+// RUN: echo "global:*ignorelisted_global*" > %t.ignorelist
+// RUN: %clang_cc1 -include %t.extra-source.cpp -fsanitize=hwaddress -fsanitize-ignorelist=%t.ignorelist -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK
----------------
aaron.ballman wrote:
> Are these files automatically deleted when the test is done because we're using %t, or do we need to clean those up manually?
AFAIK nothing is ever automatically deleted (e.g. the outputs of the compiler). Is automated cleanup here necessary?


================
Comment at: clang/test/CodeGen/hwasan-globals.cpp:9
+int global;
+int __attribute__((no_sanitize("hwaddress"))) attributed_global;
+int __attribute__((disable_sanitizer_instrumentation)) disable_instrumentation_global;
----------------
aaron.ballman wrote:
> Should we add a `memtag` test as well given that also changed in this patch?
sure, done


================
Comment at: compiler-rt/test/hwasan/TestCases/global-with-reduction.c:25
 
-int x = 1;
+#include <stdlib.h>
 
----------------
aaron.ballman wrote:
> I'm not a compiler-rt expert, but is this valid? I assume this is using the system stdlib.h which is not something we usually want in lit tests.
> 
> I think that's why `atoi` was previously being forward declared; then we don't need to include the whole header file.
I don't see there being any problem with including stdlib.h here, it's done in lots of other compiler-rt tests.

I patched this up because the compiler actually complains about forward-declaring c library functions (it's just silenced by llvm-lit by default).


================
Comment at: compiler-rt/test/hwasan/TestCases/global-with-reduction.c:50
+  f()[atoi(argv[1])] = 1;
+  return 0;
 }
----------------
aaron.ballman wrote:
> This is unnecessary -- falling off `main` already returns 0.
sure, done


================
Comment at: compiler-rt/test/hwasan/TestCases/global.c:47
   (&x)[atoi(argv[1])] = 1;
+  return 0;
 }
----------------
aaron.ballman wrote:
> This is unnecessary -- falling off `main` already returns 0.
sure, done


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127544/new/

https://reviews.llvm.org/D127544