[PATCH] D61524: [BPF] Support for compile once and run everywhere

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song marked an inline comment as done.
yonghong-song added inline comments.


================
Comment at: lib/Target/BPF/BPFAbstrctMemberAccess.cpp:522
+        Value *SrcAddr = Call->getArgOperand(BpfProbeReadSrcAddrArg);
+        if (transformProbeSrcAddr(M, SrcAddr))
+          Transformed = true;
----------------
ast wrote:
> yonghong-song wrote:
> > ast wrote:
> > > does it still need to be limited to bpf_probe_read?
> > > Shouldn't new __builtin_preserve_access_index intrinsic allow relocations to be inserted in all possible helpers?
> > Yes, the new __builtin_preserve_access_index should make BPF backend does not care about bpf_probe_read at all. The only reason I did here is for debugging. People may miss some relocatable bpf_probe_read. Using +checkoffsetreloc will print out all bpf_probe_read() calls without relocatable bpf_probe_read(). I thought this is a good debugging feature.
> > 
> > If we do not want to provide this debugging facility, the whole bpf_probe_read() things can be removed.
> it seems to be that probe_read check also gating relocation work.
> If some other call uses __builtin_preserve_access_index() it will stay un-convereted
> and will probably cause compilation failure?
> 
> It indeed makes sense to have it for debugging, but probably as a backend flag so that users can catch not only non-relocated probe_read, but other bpf helpers in the future?
> If some other call uses __builtin_preserve_access_index() it will stay un-convereted
and will probably cause compilation failure?
I think we would prefer to flag this out as they are unnecessary and may cause performance regressions.

> It indeed makes sense to have it for debugging, but probably as a backend flag so that users can catch not only non-relocated probe_read, but other bpf helpers in the future?

The current flag +checkoffsetreloc can be renamed as something like `check`. The flag is for BPF backend only. The clang also recognizes it since for CO-RE, many compilation will simply use clang only. The `check` can check for offsetreloc, but also check any future helpers, etc. Or we could have `checklevel=#` with default level 0. The higher checklevel is, the more compiler checking.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61524