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