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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
