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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8849
     break;
-  
   // HLSL attributes:
----------------
hctim wrote:
> 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
Personally, I don't feel very strongly because the chances of the whitespace being someone's entrypoint to git-blame is pretty minimal (especially given there's only one change here). However, we typically still ask for formatting changes to be separated out (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access #2) rather than lumped in with functional changes so reviewers will ask for these sort of changes to be backed out, so this may crop up repeatedly if your editor doesn't give you the options you need.


================
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
----------------
hctim wrote:
> 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?
Necessary? Probably not (I'd expect these to go into the temp directory). A kindness so disks don't fill up? Probably. Because the content here is static anyway, these could just be files in the `Inputs` directory so they don't need to be created every time the test is run.


================
Comment at: compiler-rt/test/hwasan/TestCases/global-with-reduction.c:25
 
-int x = 1;
+#include <stdlib.h>
 
----------------
hctim wrote:
> 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).
Ah, good point about other tests doing this (it's a restriction we have in Clang tests, but not here from what I can tell).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127544