[BPF] Add preserve_access_index attribute for record definition

[yonghong-song]

Summary

This patch introduced a new bpf specific attribute which can
be added to struct or union definition. For example,

struct s { ... } __attribute__((preserve_access_index));
union u { ... } __attribute__((preserve_access_index));

The goal is to simplify user codes for cases
where preserve access index happens for certain struct/union,
so user does not need to use clang __builtin_preserve_access_index
for every members.

The attribute has no effect if -g is not specified.

When the attribute is specified and -g is specified, any member
access defined by that structure or union, including array subscript
access and inner records, will be preserved through

__builtin_preserve_{array,struct,union}_access_index()

IR intrinsics, which will enable relocation generation
in bpf backend.

The following is an example to illustrate the usage:

-bash-4.4$ cat t.c
#define __reloc__ __attribute__((preserve_access_index))
struct s1 {
  int c;
} __reloc__;

struct s2 {
  union {
    struct s1 b[3];
  };
} __reloc__;

struct s3 {
  struct s2 a;
} __reloc__;

int test(struct s3 *arg) {
  return arg->a.b[2].c;
}
-bash-4.4$ clang -target bpf -g -S -O2 t.c

A relocation with access string "0:0:0:0:2:0" will be generated
representing access offset of arg->a.b[2].c.

forward declaration with attribute is also handled properly such
that the attribute is copied and populated in real record definition.

[yonghong-song]

Discussed with Martin last Friday that it would be easier for CO-RE programming if we can annotate the structure itself. This is the RFC patch. Please let me whether this sounds a reasonable approach.

[yonghong-song]

handling inner records and array subscript accesses. removing RFC tag.

[ast]

Looks great. This is a big improvement in usability.
Is there a use case to apply that attribute to inner types automatically ?

struct s1 {
  int foo;
};
struct s2 {
   struct s1 *ptr;
} __reloc__ *s2;

s2->ptr->foo -- will both deref be relocatable or only first?

[yonghong-song]

For the below:

struct s1 {
 int foo;
};
struct s2 {
 struct s1 *ptr;
} __reloc__ *s2;

s2->ptr->foo -- will both deref be relocatable or only first?
Only s2->ptr is relocated.

[ast]

Is the attribute sticky with forward delcarations?

struct s __reloc;

struct s {
  int foo;
} *s;

what is s->foo ?

[yonghong-song]

For question

Is there a use case to apply that attribute to inner types automatically ?

It is possible, but I feel make attribute per record is better. For example,

struct s1 {
 int foo;
}; 
struct s2 {
 struct s1 *ptr;
} __reloc__ *s2;

If we implicitly mark struct s1 as reloc, later user may have
another code like

struct s1 *p;
p->foo

he may be surprised by relocation.
If this is user intention to have relocation for struct s1,
I think explicit attribute is better.

Another question would be during IR generation can we magically generate relocation for ptr->foo in s2->ptr->foo? Without marking the record, this will require pass context sensitive information along code generation, which could be error prone as when to clear such context sensitive information. I would prefer explicit marking. If user uses vmlinux.h, all struct/unions in vmlinux.h could have this attribute. If user defines their own structures for later relocation, I expect this should not be a huge amount and some manual work by users should be okay.

[yonghong-song]

Is the attribute sticky with forward delcarations?

My previous comment is incorrect. It does look forward declaration can have attribution and it is sticky.

#define __reloc__ __attribute__((preserve_access_index))
struct __reloc__ p;
struct p {
 int a;
};

int test(struct p *arg) {
  return arg->a;
}
-bash-4.4$ clang -target bpf -S -O2 -emit-llvm -g t.c
-bash-4.4$ cat t.ll
...
  %0 = tail call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.ps(%struct.p*  %arg, i32 0, i32 0), !dbg !18, !llvm.pr
...

[yonghong-song]

But it does not work for inner structs any more.
-bash-4.4$ cat t.c
#define reloc attribute((preserve_access_index))
struct reloc p;
struct p {

int a;
struct {
 int b;
};

};

int test(struct p *arg) {

return arg->b;

}
-bash-4.4$

In only generates reloc for struct p's immediate members, but not inner structures.
The forward declaration gives empty record with attribute set, but this set does not
extend to inner record.

[ast]

So what happens in the following case:

struct S1;
struct S2 {
  struct S1 *f;
} relo *s2;
// access s2->f
struct S1 {
  int i;
} relo;
// access s2->f->i

lack of relo on fwd declaration is not going to be sticky 'non-relo' when it's seen later, right?
So all 3 deref will be relocatable?

[yonghong-song]

Yes, in the above, all three of them will be relocatable.

-bash-4.4$ cat t2.c
#define __reloc__ __attribute__((preserve_access_index))
 struct S1;
 struct S2 {
  struct S1 *f;
 } __reloc__;
 struct S1 {
    int i;
 } __reloc__;
 // access s2->f
 // access s2->f->i
 int test(struct S2 *arg) {
   return arg->f->i;
 }
-bash-4.4$ clang -target bpf -S -O2 -emit-llvm -g t2.c
...
  %0 = tail call %struct.S1** @llvm.preserve.struct.access.index.p0p0s_struct.S1s.p0s_struct.S2s(%struct.S2* %arg, i32 0, i32 0
), !dbg !22, !llvm.preserve.access.index !12
  %1 = load %struct.S1*, %struct.S1** %0, align 8, !dbg !22, !tbaa !23
  %2 = tail call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.S1s(%struct.S1* %1, i32 0, i32 0), !dbg !28, !llvm.pr
eserve.access.index !16

In clang, BPF routines will see the record definition if it has the attribute. So in the above, the actual definition of
struct S1 and S2 will be seen and marked with attributes properly.

[yonghong-song]

Let me take a look whether this is a *relatively easy way* to support sticky forward declaration (w.r.t. applying to inner structures).

[yonghong-song]

handle forward declaration correctly, i.e., not losing its attribute/action.

[ast]

Great. Looks like inner propagation fix was straighforward. Thanks!

[aaron.ballman]

This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc.

clang/include/clang/Basic/DiagnosticSemaKinds.td

10068

This is unnecessary.

clang/lib/Sema/SemaDeclAttr.cpp

5706

Any reason not to use a range-based for?

5712

Can use auto here

5713–5718

These should be implicit attributes, not explicit ones, right?

5725

Similar here.

5727

Can use auto here as well.

5730–5736

Implicit attributes?

5742–5747

This should be handled in Attr.td with an explicit subject.

5749

This is not needed.

[yonghong-song]

Thanks, @aaron.ballman, I will fix the issue and address all you comments. I will re-open this commit.

[yonghong-song]

The Decl "D" could be a nullptr in ProcessDeclAttributeDelayed, which will cause segfault. Handle this properly.
Also addressed most of Aaron's comments.

[yonghong-song]

use implicit attr to annotate records or fields not explicitly marked by users.

[yonghong-song]

Hi, @aaron.ballman I think I addressed all your comments, could you take a look again? I tested all failed tests as exposed by the buildbot here (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/57856). They are all passing now. Thanks!

[yonghong-song]

Regarding to this one "This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc."
Could you give me some example tests which I can take a look in order to add support. FYI, BPF backend publicly only supports C language,
(a restrict C for example __thread keyword is not supported). I guess template instantiations does not apply here? Or we can still test
template instantiation since we are at clang stage?

[ast]

I've tested this patch on several kernel selftests/bpf/ and it works like magic. Very nice improvement.

[yonghong-song]

@aaron.ballman BTW, I indeed tested C-style inheritance. An attribute for the forward declaration successfully inherited by a later actual declaration.

[yonghong-song]

@aaron.ballman just ping, could you let me know if you have any further comments? Thanks!

[aaron.ballman]

Why did this get committed when there were still outstanding review questions that were not answered?

In D69759#1739935, @yonghong-song wrote:

Regarding to this one "This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc."
Could you give me some example tests which I can take a look in order to add support. FYI, BPF backend publicly only supports C language,
(a restrict C for example __thread keyword is not supported). I guess template instantiations does not apply here? Or we can still test
template instantiation since we are at clang stage?

If the attribute only makes sense in C mode, it should not be accepted in C++ mode. However, it currently is accepted in C++ mode which is why I was asking for tests demonstrating how it should behave.

In D69759#1742264, @yonghong-song wrote:

@aaron.ballman just ping, could you let me know if you have any further comments? Thanks!

I was out all last week at wg21 meetings and a short vacation, so that's why the delay in review. FWIW, we usually give a week before pinging a review.

clang/include/clang/Basic/Attr.td

1584

If this attribute is only supported in C mode, then this attribute should have: let LangOpts = [COnly]; in it.

clang/lib/CodeGen/CGExpr.cpp

3411–3422

Is there a reason to do this as opposed to IgnoreImpCasts()?

3423–3424

if (const auto *ME = dyn_cast<MemberExpr>(E))
  return ...

3427–3428

Similar here -- you can sink the variable into the if.

3433

const auto *PtrT = VarDef->getType()->getAs<PointerType>();

3440–3443

Is there a reason you only want to strip off typedefs, and not other forms of sugar? e.g., why not call getUnqualifiedDesugaredType()?

3446

const auto *

[yonghong-song]

Sorry about the committing. I thought all the review questions are addressed.
Definitely will be more patient next time until getting review confirmation and
also adhering to llvm review policy.
Thanks for the detailed review! I have addressed all of your review comments in patch
https://reviews.llvm.org/D70257. I have added you as the reviewer.

clang/include/clang/Basic/Attr.td

1584

Yes, added.

clang/lib/CodeGen/CGExpr.cpp

3440–3443

Actually, ElaboratedT is also a sugar type, the above getUnqualifiedDesugaredType() will remove it too.

[aaron.ballman]

In D69759#1746192, @yonghong-song wrote:

Sorry about the committing. I thought all the review questions are addressed.

No worries, this happens sometimes. :-)

Definitely will be more patient next time until getting review confirmation and
also adhering to llvm review policy.

Thanks!

Thanks for the detailed review! I have addressed all of your review comments in patch
https://reviews.llvm.org/D70257. I have added you as the reviewer.

Fantastic, thank you!

clang/lib/CodeGen/CGExpr.cpp

3440–3443

Yes, but you just strip off that sugar anyway, so that's why I suggested this. It seems you got the gist of my suggestion in the other review though.

clang/include/clang/Basic/Attr.td

class TargetSpec {
   code CustomCode = [{}];
 }
 
 class TargetArch<list<string> arches> : TargetSpec {
   let Arches = arches;
 }
 def TargetARM : TargetArch<["arm", "thumb", "armeb", "thumbeb"]>;
 def TargetAVR : TargetArch<["avr"]>;
+def TargetBPF : TargetArch<["bpfel", "bpfeb"]>;
 def TargetMips32 : TargetArch<["mips", "mipsel"]>;
 def TargetAnyMips : TargetArch<["mips", "mipsel", "mips64", "mips64el"]>;
 def TargetMSP430 : TargetArch<["msp430"]>;
 def TargetRISCV : TargetArch<["riscv32", "riscv64"]>;
 def TargetX86 : TargetArch<["x86"]>;
 def TargetAnyX86 : TargetArch<["x86", "x86_64"]>;
 def TargetWebAssembly : TargetArch<["wasm32", "wasm64"]>;
 def TargetWindows : TargetArch<["x86", "x86_64", "arm", "thumb", "aarch64"]> {

 
 def AMDGPUNumVGPR : InheritableAttr {
   let Spellings = [Clang<"amdgpu_num_vgpr", 0>];
   let Args = [UnsignedArgument<"NumVGPR">];
   let Documentation = [AMDGPUNumSGPRNumVGPRDocs];
   let Subjects = SubjectList<[Function], ErrorDiag, "kernel functions">;
 }
 
+def BPFPreserveAccessIndex : InheritableAttr,
+                             TargetSpecificAttr<TargetBPF>  {
+  let Spellings = [Clang<"preserve_access_index">];
+  let Documentation = [BPFPreserveAccessIndexDocs];
+}
+
 def WebAssemblyImportModule : InheritableAttr,
                               TargetSpecificAttr<TargetWebAssembly> {
   let Spellings = [Clang<"import_module">];
   let Args = [StringArgument<"ImportModule">];
   let Documentation = [WebAssemblyImportModuleDocs];
   let Subjects = SubjectList<[Function], ErrorDiag>;
 }
 

clang/include/clang/Basic/AttrDocs.td

- If the CPU is not M-class, the return instruction is changed to one of the
 
   Unfortunately the compiler is unable to make this guarantee for an "UNDEF"
   handler, where the offset from "lr" to the preferred return address depends on
   the execution state of the code which generated the exception. In this case
   a sequence equivalent to "movs pc, lr" will be used.
   }];
 }
 
+def BPFPreserveAccessIndexDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Clang supports the ``__attribute__((preserve_access_index))``
+attribute for the BPF target. This attribute may be attached to a
+struct or union declaration, where if -g is specified, it enables
+preserving struct or union member access debuginfo indicies of this
+struct or union, similar to clang ``__builtin_preserve_acceess_index()``.
+  }];
+}
+
 def MipsInterruptDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "interrupt (MIPS)";
   let Content = [{
 Clang supports the GNU style ``__attribute__((interrupt("ARGUMENT")))`` attribute on
 MIPS targets. This attribute may be attached to a function definition and instructs
 the backend to generate appropriate function entry/exit code so that it can be used
 directly as an interrupt service routine.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_builtin_launder_invalid_arg : Error<
   "%select{non-pointer|function pointer|void pointer}0 argument to "
   "'__builtin_launder' is not allowed">;
 
 def err_preserve_field_info_not_field : Error<
   "__builtin_preserve_field_info argument %0 not a field access">;
 def err_preserve_field_info_not_const: Error<
   "__builtin_preserve_field_info argument %0 not a constant">;
+def err_preserve_access_index_wrong_type: Error<"%0 attribute only applies to %1">;
 
 def err_bit_cast_non_trivially_copyable : Error<
   "__builtin_bit_cast %select{source|destination}0 type must be trivially copyable">;
 def err_bit_cast_type_size_mismatch : Error<
   "__builtin_bit_cast source size does not equal destination size (%0 vs %1)">;
 } // end of sema component.

clang/lib/CodeGen/CGExpr.cpp

 }
 
 static Address emitArraySubscriptGEP(CodeGenFunction &CGF, Address addr,
                                      ArrayRef<llvm::Value *> indices,
                                      QualType eltType, bool inbounds,
                                      bool signedIndices, SourceLocation loc,
                                      QualType *arrayType = nullptr,
                                      const llvm::Twine &name = "arrayidx") {
   // All the indices except that last must be zero.
 #ifndef NDEBUG
   for (auto idx : indices.drop_back())
     assert(isa<llvm::ConstantInt>(idx) &&
            cast<llvm::ConstantInt>(idx)->isZero());
 #endif
 
   // Determine the element size of the statically-sized base.  This is
   // the thing that the indices are expressed in terms of.
   if (auto vla = CGF.getContext().getAsVariableArrayType(eltType)) {
     eltType = getFixedSizeElementType(CGF.getContext(), vla);
   }
 
   // We can use that to compute the best alignment of the element.
   CharUnits eltSize = CGF.getContext().getTypeSizeInChars(eltType);
   CharUnits eltAlign =
     getArrayElementAlign(addr.getAlignment(), indices.back(), eltSize);
 
   llvm::Value *eltPtr;
   auto LastIndex = dyn_cast<llvm::ConstantInt>(indices.back());
   if (!CGF.IsInPreservedAIRegion || !LastIndex) {
     eltPtr = emitArraySubscriptGEP(
         CGF, addr.getPointer(), indices, inbounds, signedIndices,
         loc, name);
   } else {
     // Remember the original array subscript for bpf target
     unsigned idx = LastIndex->getZExtValue();
     llvm::DIType *DbgInfo = nullptr;
     if (arrayType)
       DbgInfo = CGF.getDebugInfo()->getOrCreateStandaloneType(*arrayType, loc);
     eltPtr = CGF.Builder.CreatePreserveArrayAccessIndex(addr.getPointer(),
                                                         indices.size() - 1,
                                                         idx, DbgInfo);
   }
 
   return Address(eltPtr, eltAlign);
 }
 
 LValue CodeGenFunction::EmitArraySubscriptExpr(const ArraySubscriptExpr *E,
                                                bool Accessed) {
   // The index must always be an integer, which is not an aggregate.  Emit it
   // in lexical order (this complexity is, sadly, required by C++17).
   llvm::Value *IdxPre =
       (E->getLHS() == E->getIdx()) ? EmitScalarExpr(E->getIdx()) : nullptr;
LValue CodeGenFunction::EmitLValueForField(LValue base,
   LValueBaseInfo BaseInfo = base.getBaseInfo();
 
   if (field->isBitField()) {
     const CGRecordLayout &RL =
       CGM.getTypes().getCGRecordLayout(field->getParent());
     const CGBitFieldInfo &Info = RL.getBitFieldInfo(field);
     Address Addr = base.getAddress();
     unsigned Idx = RL.getLLVMFieldNo(field);
-    if (!IsInPreservedAIRegion) {
+    const RecordDecl *rec = field->getParent();
+    if (!IsInPreservedAIRegion &&
+        (!getDebugInfo() || !rec->hasAttr<BPFPreserveAccessIndexAttr>())) {
       if (Idx != 0)
         // For structs, we GEP to the field that the record layout suggests.
         Addr = Builder.CreateStructGEP(Addr, Idx, field->getName());
     } else {
-      const RecordDecl *rec = field->getParent();
       llvm::DIType *DbgInfo = getDebugInfo()->getOrCreateRecordType(
           getContext().getRecordType(rec), rec->getLocation());
       Addr = Builder.CreatePreserveStructAccessIndex(Addr, Idx,
           getDebugInfoFIndex(rec, field->getFieldIndex()),
           DbgInfo);
     }
 
     // Get the access type.
if (rec->isUnion()) {
     // For unions, there is no pointer adjustment.
     if (CGM.getCodeGenOpts().StrictVTablePointers &&
         hasAnyVptr(FieldType, getContext()))
       // Because unions can easily skip invariant.barriers, we need to add
       // a barrier every time CXXRecord field with vptr is referenced.
       addr = Address(Builder.CreateLaunderInvariantGroup(addr.getPointer()),
                      addr.getAlignment());
 
-    if (IsInPreservedAIRegion) {
+    if (IsInPreservedAIRegion ||
+        (getDebugInfo() && rec->hasAttr<BPFPreserveAccessIndexAttr>())) {
       // Remember the original union field index
       llvm::DIType *DbgInfo = getDebugInfo()->getOrCreateRecordType(
           getContext().getRecordType(rec), rec->getLocation());
       addr = Address(
           Builder.CreatePreserveUnionAccessIndex(
               addr.getPointer(), getDebugInfoFIndex(rec, field->getFieldIndex()), DbgInfo),
           addr.getAlignment());
     }
 
     if (FieldType->isReferenceType())
       addr = Builder.CreateElementBitCast(
           addr, CGM.getTypes().ConvertTypeForMem(FieldType), field->getName());
   } else {
-    if (!IsInPreservedAIRegion)
+    if (!IsInPreservedAIRegion &&
+        (!getDebugInfo() || !rec->hasAttr<BPFPreserveAccessIndexAttr>()))
       // For structs, we GEP to the field that the record layout suggests.
       addr = emitAddrOfFieldStorage(*this, addr, field);
     else
       // Remember the original struct field index
       addr = emitPreserveStructAccess(*this, addr, field);
   }
 
   // If this is a reference field, load the reference right now.

clang/lib/Sema/SemaDeclAttr.cpp

static void handleAVRSignalAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   }
 
   if (!checkAttributeNumArgs(S, AL, 0))
     return;
 
   handleSimpleAttribute<AVRSignalAttr>(S, D, AL);
 }
 
+static void handleBPFPreserveAccessIndex(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (!isa<RecordDecl>(D)) {
+    S.Diag(D->getLocation(), diag::err_preserve_access_index_wrong_type)
+        << "preserve_addess_index" << "struct or union type";
+    return;
+  }
+
+  if (!checkAttributeNumArgs(S, AL, 0))
+    return;
+
+  D->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, AL));
+}
+
 static void handleWebAssemblyImportModuleAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!isFunctionOrMethod(D)) {
     S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
         << "'import_module'" << ExpectedFunction;
     return;
   }
 
   auto *FD = cast<FunctionDecl>(D);
   if (FD->isThisDeclarationADefinition()) {
     S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
     return;
   }
 
   StringRef Str;
   SourceLocation ArgLoc;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &ArgLoc))
     return;
 
   FD->addAttr(::new (S.Context)
                   WebAssemblyImportModuleAttr(S.Context, AL, Str));
 }
 
 static void handleWebAssemblyImportNameAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!isFunctionOrMethod(D)) {
     S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
         << "'import_name'" << ExpectedFunction;
     return;
   }
 
   auto *FD = cast<FunctionDecl>(D);
   if (FD->isThisDeclarationADefinition()) {
     S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
     return;
   }
 
   StringRef Str;
   SourceLocation ArgLoc;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &ArgLoc))
     return;
 
   FD->addAttr(::new (S.Context) WebAssemblyImportNameAttr(S.Context, AL, Str));
 }
case ParsedAttr::AT_AMDGPUNumSGPR:
     handleAMDGPUNumSGPRAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AMDGPUNumVGPR:
     handleAMDGPUNumVGPRAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AVRSignal:
     handleAVRSignalAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_BPFPreserveAccessIndex:
+    handleBPFPreserveAccessIndex(S, D, AL);
+    break;
   case ParsedAttr::AT_WebAssemblyImportModule:
     handleWebAssemblyImportModuleAttr(S, D, AL);
     break;
   case ParsedAttr::AT_WebAssemblyImportName:
     handleWebAssemblyImportNameAttr(S, D, AL);
     break;
   case ParsedAttr::AT_IBAction:
     handleSimpleAttribute<IBActionAttr>(S, D, AL);