[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

[Eduard Zingerman via Phabricator via cfe-commits]

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