@aeubanks Thanks for the background information. Please feel free to make changes so that BPFAbstractMemberAccess.cpp does not use getPointerElementType. For example, if needed you could extend IR intrinsics to pass additional information from clang frontend codegen to llvm. I am happy to do a code review then.

In D61524#2798168, @aeubanks wrote:

Sorry to come to this years later, but the new intrinsics look at the base pointer's pointee type to determine size. The whole point of the opaque pointers transition is that pointee types are meaningless (this was sorta mentioned in the review comments). GEP instructions have an explicit type as part of the instruction to know what type the base pointer is pointing to. I think it's a bit harder to associate intrinsics with a type. We could have a dummy argument with an overloaded type, but that's a bit weird. Any ideas?

In D103667#2798165, @dblaikie wrote:

Is the C source syntax already fixed? Could it use a more specific name - then maybe wouldn't need the flag to opt-in if the only places the attribute appears is where it should be propagated?

@MaskRay Did you get some time to look at the new revision?

@dblaikie Thanks for the pointer! I will work on the new patch, starting with DW_AT_BTF_annotation (to limit the scope where annotations will be processed) and post a new patch soon.

As for supporting it in DWARF, probably with a custom attribute (DW_AT_BTF_annotation? (or "LLVM" instead of "BTF" perhaps, I'm not sure)) with a standard form (DW_FORM_strp/strxN/etc - the usual way we emit strings).

• https://reviews.llvm.org/D102712 made llvm BPF relocations more lld friendly. The patch has landed. Now redo the lld patch with new relocations.
• Per early Fangrui's suggestion, I submitted the following llvm relocation patch https://lore.kernel.org/bpf/20210525033314.3008878-1-yhs@fb.com/ to linux kernel mailing list and if accepted it will be in kernel bpf docs.

• add one test case to generate R_BPF_64_ABS64 relocation for global data.

• enumerate all relocations in RuntimeDyld.c such that R_BPF_64_{ABS64,ABS32} relocations will be resolved and others will be ignored.
• added one more test case that R_BPF_64_ABS64 is properly resolved.

• use FK_SecRel_8 Fixup kind for LD_imm64 operands instead of FK_SecRel_4.

• add a test for ExecutionEngine/RuntimeDyld for added BPF relocation case. Without the patch in RuntimeDyldELF.cpp, the test will crash
• rename R_BPF_64_ALLOC32 to R_BPF_64_NODYLD32.
• added @MaskRay as the reviewer
• remaining known issues to be resolved:
  • can BPF backend use FK_SecRel_4 for R_BPF_64_64?
  • Is the relocation name R_BPF_64_NODYLD32 okay or we could have a better one?

• rename R_BPF_64_D{32,64} to R_BPF_64_ABS{32,64} to better reflect the intention of relocation. *ABS{32,64} names have been used by arm, aarch64 and amdgpu.

• removed FK_SecRel_8 and used FK_SecRel_4 for LD_imm64 fixup kind which reflects better that fact only the first 4-byte insn->off is allowed for storing the section offset.

• add more comments and descriptions for relocations.

In D102712#2767238, @MaskRay wrote:

These ELF flags should be documented somewhere. Even a draft will help.

For example, riscv is using https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#relocations , though not as sophisticated as other psABIs (x86-64,aarch64,ppc64 ELFv2)

Discussed with Alexei, we are going to add more relocations in BPF backend. Here is the patch https://reviews.llvm.org/D102712. You can take a look if you are interested.
Once it or its variant is merged, I will remove getImplicitAddendAlloc() and relocateAlloc(). Then we will have no non-target changes except disabling -ffunction-sections for BPF LTO relocatable mode. Thanks!

This patch will require bpf-next libbpf patch like below:

diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index ee426226928f..ccc5d4591801 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -28,6 +28,12 @@
 #ifndef R_BPF_64_64
 #define R_BPF_64_64 1
 #endif
+#ifndef R_BPF_64_LD64
+#define R_BPF_64_LD64 2
+#endif
+#ifndef R_BPF_64_PC32
+#define R_BPF_64_PC32 3
+#endif
 #ifndef R_BPF_64_32
 #define R_BPF_64_32 10
 #endif
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index b594a88620ce..3e3cd64025dd 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -892,7 +892,8 @@ static int linker_sanity_check_elf_relos(struct src_obj *obj, struct src_sec *se
                size_t sym_idx = ELF64_R_SYM(relo->r_info);
                size_t sym_type = ELF64_R_TYPE(relo->r_info);

@alessandrod FYI. This is to support BPF in lld. Maybe this may help your rust bpf-linker work?

• removed hack in InputSection.cpp to prevent emitting relocations for relocatable BPF.
• added two new target hooks to differentiate between ALLOC and non-ALLOC as BPF targets need them to take different actions.
• implement target relocate*() and getImplicitAddend*() properly.

For

I am also very concerned with potentially other differently interpreted concepts, e.g. the re-defined STV_INTERNAL (https://lists.llvm.org/pipermail/cfe-dev/2021-May/068134.html).

This would potentially diverge from other implementations and add some symbol resolution complexity to the generic code (say, we would have to check STV_INTERNAL in Symbols.cpp just for BPF)

For this Why is this <unknown>? if you remove if (config->emachine != EM_BPF && type != target->noneRel), this is because I didn't implement BPF target relocate properly. In current llvm BPF backend, BPFAsmBackend.cpp file, we have

void BPFAsmBackend::applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                               const MCValue &Target,
                               MutableArrayRef<char> Data, uint64_t Value,
                               bool IsResolved,
                               const MCSubtargetInfo *STI) const {
  if (Fixup.getKind() == FK_SecRel_4 || Fixup.getKind() == FK_SecRel_8) {
    // The Value is 0 for global variables, and the in-section offset
    // for static variables. Write to the immediate field of the inst.
    assert(Value <= UINT32_MAX);
    support::endian::write<uint32_t>(&Data[Fixup.getOffset() + 4],
                                     static_cast<uint32_t>(Value),
                                     Endian);
  } else if (Fixup.getKind() == FK_Data_4) {
    support::endian::write<uint32_t>(&Data[Fixup.getOffset()], Value, Endian);
  } else if (Fixup.getKind() == FK_Data_8) {
    support::endian::write<uint64_t>(&Data[Fixup.getOffset()], Value, Endian);
  } else if (Fixup.getKind() == FK_PCRel_4) {
    Value = (uint32_t)((Value - 8) / 8);
    if (Endian == support::little) {
      Data[Fixup.getOffset() + 1] = 0x10;
      support::endian::write32le(&Data[Fixup.getOffset() + 4], Value);
    } else {
      Data[Fixup.getOffset() + 1] = 0x1;
      support::endian::write32be(&Data[Fixup.getOffset() + 4], Value);
    } 
  } else {
    assert(Fixup.getKind() == FK_PCRel_2);
    Value = (uint16_t)((Value - 8) / 8);
    support::endian::write<uint16_t>(&Data[Fixup.getOffset() + 2], Value,
                                     Endian);
  }                                  
}

Sure. I will merge the patch for you.

LGTM except one minor issue. Please do address the nit before merging. Thanks!

• Addressed @MaskRay's comments. Thanks for quick reviewing!

handling static functions (int a different section) properly. static variables should be similar.

add tests/docs and target hook emitFuncDataSections(). Removed WIP tag.
@MaskRay Could you take a look? Thanks!

Generally looks good. One suggestion and a few nits. Thanks!

That is true that bar[1] is not an field. We also support field relocation like bar[1] since if sizeof(bar[1]) changed we can adjust offset properly. So there is
a precedence to support array like bar[1] for *field* relocations. So I think supporting "bar[1]" for existence relocation is not a bad thing.

Ok, sounds good then.

This looks better. Do you have a plan to eventually remove this fix as it really looks incompatible with other platforms?

Toolchain can choose whether they want to override the default handling of triple. AIX toolchain choose not to override and use the default one.

Maybe it is better to do what other platform is doing? What is the advantage of not overriding the default triple for not supported platform?

If I understand correctly, on AIX system, BPF is not supported, so if -march=bpf is specified, it will internally expand to -mtriple=bpfel-ibm-aix and the test will fail since it is not supported, if llc uses -mtriple=bpfel and then llvm understands it is not really related to AIX and it is fine, is that right?

We have a lot of tests using -march=bpf[eleb], why only these two tests? Is this due to the fact llvm-objdump won't work any more? Also, we have quite some objdump-* tests,

$ ls objdump*.ll
objdump_atomics.ll    objdump_cond_op.ll  objdump_imm_hex.ll     objdump_nop.ll         objdump_trivial.ll
objdump_cond_op_2.ll  objdump_dis_all.ll  objdump_intrinsics.ll  objdump_static_var.ll  objdump_two_funcs.ll

They all have similar llc | llvm-objdump | FileCheck test commands? Do other objdump_* tests also need to change to triple?

formatting to mode "DI = CGM.getModuleDebugInfo()" and use cast<...> instead of dyn_cast<...> since there should be no possibility of null ptr.

• use stripPointerCasts() to remove the cast of DeclRefExpr pointer

Looks like stripPointerCasts() is enough to make it work (no null Function pointer any more). This is consistent with

llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
    StringRef MangledName, llvm::Type *Ty, GlobalDecl GD, bool ForVTable,
    bool DontDefer, bool IsThunk, llvm::AttributeList ExtraAttrs,
    ForDefinition_t IsForDefinition) {
  const Decl *D = GD.getDecl();
...
  // Make sure the result is of the requested type.
  if (!IsIncompleteFunction) {
    assert(F->getFunctionType() == Ty);
    return F;
  }

• somehow previous test case didn't trigger nullptr for Fn. Replaced it with a working test case.

With the following change,

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index a784aade88da..fc86c1a72056 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -2841,6 +2841,7 @@ LValue CodeGenFunction::EmitDeclRefLValue(const DeclRefExpr *E) {
     if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
       CGDebugInfo *DI = CGM.getModuleDebugInfo();
       auto *Fn = dyn_cast<llvm::Function>(LV.getPointer(*this));
+      fprintf(stderr, "Fn = %p\n", (void *)Fn);
       if (DI && Fn && !Fn->getSubprogram())
         DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
     }

I have the following for the added test case,

$ clang -cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm debug-info-extern-callback.c
Fn = 0x7afcf28
Fn = 0x7afce98
Fn = (nil)

@dblaikie ping. Did you get a chance of looking at it?

ping @dblaikie could you take a look of my new investigation?

@dblaikie I did some investigation, found the root cause and provided an alternative solution. Could you check whether you are okay with either approach?

Did some further investigation, the following is the callchain:

EmitFunctionDeclLValue
   EmitFunctionDeclPointer
       GetAddrOfFunction
           GetOrCreateLLVMFunction

To answer one of your questions above, if there is a function definition before, dyn_cast<llvm::Function>(...) will return nullptr. I tried starting from "Value" class and found dyn_cast to llvm::ConstantExpr will result in a non-null object, but I did not trace down further with subclasses of llvm::ConstantExpr.

In D100567#2697412, @dblaikie wrote:

In D100567#2696095, @yonghong-song wrote:

Sorry, I know what is the segfault now after some debugging. It is because auto *Fn = dyn_cast<llvm::Function>(LV.getPointer(*this)); is a NULL pointer after there is a definition before this.

Hmm, not sure I follow - is LV.getPointer(*this) returning null, or is the dyn_cast returning null because the thing isn't an llvm::Function? If it's not a function, what is it?

I did some debugging on why DeclRefExpr evaluated not to be a Function pointer. Here is what I got.
I made the following clang change to dump out the pointer to the emited value ('&foo'):

• add checking CGDebugInfo pointer. In my original patch but got lost in the revision.

• isDefined() is not necessary, segfault is caused by a nullptr by casting a LV.getPointer(). Check nullptr instead.

Sorry, I know what is the segfault now after some debugging. It is because auto *Fn = dyn_cast<llvm::Function>(LV.getPointer(*this)); is a NULL pointer after there is a definition before this. Will pose another patch but with test case remains to cover mix of declaration, pointer reference and definition.

In D100567#2695827, @dblaikie wrote:

Generally looks OK, but could you explain more about the problems with/motivation for the declaration/definition issue? (probably worth noting in the patch description maybe comment in code and test)

@dblaikie I renamed the variable, added a few guards to ensure safety and avoid unnecessary attempt to generate duplicated debuginfo, and enhanced the test case to cover mix of declaration, accessing extern func address and definition of the function. Could you take a look?

fix a clang-format warning

• checked both !FD->isDefined() and !Fn->getSubprogram() before emitting debuginfo for the declaration
• added additional tests to mix DeclRefExpr and actual function definition.

• check Fn->getSubprogram() before emit debuginfo. This is consistent with EmitFuncDeclForCallSite() and is better than checking FD->isDefined() as we may hit multiple DeclRefExpr and all of them will go through emitting debuginfo.

check FD->isDefined() as well before emit debuginfo for the declaration. It is okay to emit a declaration subprogram and later refined to be with definition. But it is not okay to refine a definition to a declaration. So add check here.

• change the test case to have a section name for the extern func which should be the most common use case. We already have tests for extern func (direct call instead of callback) which called the same internal llvm function, so the case without section name should be covered as well.

Rename TargetInfo.allowDebugInfoForExternalVar to TargetInfo.allowDebugInfoForExternalRef.

For the first example, actually clang is smart enough to remove all dead code, so nothing generated.

Yes.

The corresponding clang patch is here: https://reviews.llvm.org/D100567

The corresponding llvm patch is here: https://reviews.llvm.org/D100568