eddyz87 added a comment.
In D133361#4652368 <https://reviews.llvm.org/D133361#4652368>, @erichkeane
wrote:
> I don't see the comment response you had to me.
Sorry, forgot to click submit.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7601
+ auto *Rec = cast<RecordDecl>(D);
+ Rec->addAttr(::new (S.Context) BPFPreserveStaticOffsetAttr(S.Context, AL));
+}
----------------
erichkeane wrote:
> This should use `BPFPreserveStaticOffsetAttr::Create` instead of using
> placement `new`. We've been meaning to translate these all at one point, but
> never got around to it.
Replaced by a call to `handleSimpleAttribute` as suggested by Aaron.
================
Comment at: clang/test/Sema/bpf-attr-preserve-static-offset.c:1
+// RUN: %clang_cc1 -fsyntax-only -ast-dump -triple bpf-pc-linux-gnu %s |
FileCheck %s
+
----------------
aaron.ballman wrote:
> You should also add a `-verify` test that verifies we diagnose applying the
> attribute to something other than a structure or union, accepts no arguments,
> is diagnosed on a non-BPF target, etc to ensure we've got correct diagnostic
> behavior.
`-verify` looks neat, thank you.
I've added two test cases:
- clang/test/Sema/bpf-attr-preserve-static-offset-warns.c
- clang/test/Sema/bpf-attr-preserve-static-offset-warns-nonbpf.c
Those check for things you listed:
- can't be used for non-bpf target
- can't take parameters
- is error to put it on something other than struct / union.
Tbh, I don't know if there is anything else to test in this regard.
================
Comment at: llvm/test/CodeGen/BPF/preserve-static-offset/load-align.ll:61
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 18.0.0 (/home/eddy/work/llvm-project/clang
c899a1ca75d0f1b559204eff79a2578d2cafc7ab)"}
+!2 = !{!3, !4, i64 128}
----------------
erichkeane wrote:
> Are we sure we want to do something like this? It seems this both depends on
> YOUR computer AND us never releasing Clang 18.
Are you sure this would be an issue?
The specific line is not a part of a CHECK and I tried the following command
using my system's llvm 16 opt:
```
opt -O2 -mtriple=bpf-pc-linux -S -o - load-align.ll
```
And module was loaded / processed w/o any issues.
In general grepping shows that people don't usually mask these in tests:
```
$ cd llvm/test/CodeGen/
$ ag '{!"clang version' | wc -l
452
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133361/new/
https://reviews.llvm.org/D133361
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
