This is an archive of the discontinued LLVM Phabricator instance.

[BPF] Fix a bug in BPFMISimplifyPatchable pass
ClosedPublic

Authored by yonghong-song on Apr 15 2022, 4:16 PM.

Details

Summary

LLVM BPF pass SimplifyPatchable is used to do necessary
code conversion for CO-RE operations. When studying bpf
selftest 'exhandler', I found a corner case not handled properly.
The following is the C code, modified from original 'exhandler'
code.

int g;
int test(struct t1 *p) {
  struct t2 *q = p->q;
  if (q)
    return 0;
  struct t3 *f = q->f;
  if (!f) g = 5;
  return 0;
}

For code:

struct t3 *f = q->f;
if (!f) ...

The IR before BPFMISimplifyPatchable pass looks like:

%5:gpr = LD_imm64 @"llvm.t2:0:8$0:1"
%6:gpr = LDD killed %5:gpr, 0
%7:gpr = LDD killed %6:gpr, 0
JNE_ri killed %7:gpr, 0, %bb.3
JMP %bb.2

Note that compiler knows q = 0 based dataflow and value analysis.
The correct generated code after the pass should be

%5:gpr = LD_imm64 @"llvm.t2:0:8$0:1"
%7:gpr = LDD killed %5:gpr, 0
JNE_ri killed %7:gpr, 0, %bb.3
JMP %bb.2

But the current implementation did further optimization for the
above code and generates

%5:gpr = LD_imm64 @"llvm.t2:0:8$0:1"
JNE_ri killed %5:gpr, 0, %bb.3
JMP %bb.2

which is incorrect.

This patch added a cache to remember those load insns not associated
with CO-RE offset value and will skip these load insns during
transformation.

Diff Detail

Event Timeline

yonghong-song created this revision.Apr 15 2022, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 4:16 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
yonghong-song requested review of this revision.Apr 15 2022, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 4:16 PM
ast added inline comments.Apr 19 2022, 10:31 AM
llvm/lib/Target/BPF/BPFMISimplifyPatchable.cpp
243

why gate only loads here?
The hunk below will apply to loads only anyway,
so this check looks unnecessary.
If both filter_loads checks (here and line 267) are necessary may be combine them into a helper function?

yonghong-song added inline comments.Apr 19 2022, 1:25 PM
llvm/lib/Target/BPF/BPFMISimplifyPatchable.cpp
243

They are not redundant. We have SkipInsts.insert(Inst); here.
But a helper function (here and line 267) is a good idea. I kind of realize it but a little bit lazy, but since you mentioned this. Will do it.

  • create and use isLoadInst() helper function.
ast accepted this revision.Apr 19 2022, 2:57 PM
This revision is now accepted and ready to land.Apr 19 2022, 2:57 PM
This revision was landed with ongoing or failed builds.Apr 19 2022, 3:24 PM
This revision was automatically updated to reflect the committed changes.