[PATCH] D83878: BPF: support type exist/size and enum exist/value relocations

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song added inline comments.


================
Comment at: llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp:901
+    if (CInfo.AccessIndex == BPFCoreSharedInfo::ENUM_VALUE) {
+      PatchImm = getConstant(IntVal);
+    } else {
----------------
anakryiko wrote:
> shouldn't PatchImm be a 64-bit value?
I kind of lazy for this and that is why I have 32bit for now. Yes, we can have 64bit value. Will need to change clang builtin return value to 64bit int and then in llvm we will store 64bit value instead of 32bit.



================
Comment at: llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp:904
+      PatchImm = 1;
+      AccessStrVal = getConstant(IntVal);
+    }
----------------
anakryiko wrote:
> so if I understand this correctly, access string will encode the integer value of ENUM_VAL, is that right? If yes, that's a bit problematic, because it's ambiguous. Multiple ENUM_VALs can have the same integer value, but we actually want to capture which of those values have to be checked/relocated. Is it possible to record enumerator index, i.e.:
> 
> for enum ENUM {
>     ENUM_VAL1 = 123,
>     ENUM_VAL2 = 123,
> }
> 
> we record either 0 for ENUM_VAL1 or 1 for ENUM_VAL2, not 123?
This is a good point. I missed this point. The ENUM_VAL1 information is lost in IR and only available in clang frontend. Will make necessary changes to have enumerator string encoded instead of just int number.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83878/new/

https://reviews.llvm.org/D83878