[PATCH, bpf-llvm] Fix bug on silently truncating 64-bit immediate


Jiong Wang
 

We came across an llvm bug when compiling some testcases that 64-bit
immediates are silently truncated into 32-bit and then packed into
BPF_JMP | BPF_K encoding. This caused comparison with wrong value.

This bug looks to be introduced by r308080. The Select_Ri pattern is
supposed to be lowered into J*_Ri while the latter only support 32-bit
immediate encoding, therefore Select_Ri should have similar immediate
predicate check as what J*_Ri are doing.

Reported-by: Jakub Kicinski <jakub.kicinski@...>
Signed-off-by: Jiong Wang <jiong.wang@...>
---
lib/Target/BPF/BPFISelLowering.cpp | 8 ++++++--
lib/Target/BPF/BPFInstrInfo.td | 2 +-
test/CodeGen/BPF/select_ri.ll | 35 +++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/lib/Target/BPF/BPFISelLowering.cpp b/lib/Target/BPF/BPFISelLowering.cpp
index d4e06dd..995f206 100644
--- a/lib/Target/BPF/BPFISelLowering.cpp
+++ b/lib/Target/BPF/BPFISelLowering.cpp
@@ -611,11 +611,15 @@ BPFTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
.addReg(LHS)
.addReg(MI.getOperand(2).getReg())
.addMBB(Copy1MBB);
- else
+ else {
+ int64_t imm32 = MI.getOperand(2).getImm();
+ // sanity check before we build J*_ri instruction.
+ assert (isInt<32>(imm32));
BuildMI(BB, DL, TII.get(NewCC))
.addReg(LHS)
- .addImm(MI.getOperand(2).getImm())
+ .addImm(imm32)
.addMBB(Copy1MBB);
+ }

// Copy0MBB:
// %FalseValue = ...
diff --git a/lib/Target/BPF/BPFInstrInfo.td b/lib/Target/BPF/BPFInstrInfo.td
index fcd6a60..a3ad2ee 100644
--- a/lib/Target/BPF/BPFInstrInfo.td
+++ b/lib/Target/BPF/BPFInstrInfo.td
@@ -460,7 +460,7 @@ let usesCustomInserter = 1 in {
(ins GPR:$lhs, i64imm:$rhs, i64imm:$imm, GPR:$src, GPR:$src2),
"# Select PSEUDO $dst = $lhs $imm $rhs ? $src : $src2",
[(set i64:$dst,
- (BPFselectcc i64:$lhs, (i64 imm:$rhs), (i64 imm:$imm), i64:$src, i64:$src2))]>;
+ (BPFselectcc i64:$lhs, (i64immSExt32:$rhs), (i64 imm:$imm), i64:$src, i64:$src2))]>;
}

// load 64-bit global addr into register
diff --git a/test/CodeGen/BPF/select_ri.ll b/test/CodeGen/BPF/select_ri.ll
index b802b64..7b1f852 100644
--- a/test/CodeGen/BPF/select_ri.ll
+++ b/test/CodeGen/BPF/select_ri.ll
@@ -25,3 +25,38 @@ entry:
}

attributes #0 = { norecurse nounwind readonly }
+
+; test immediate out of 32-bit range
+; Source file:
+
+; unsigned long long
+; load_word(void *buf, unsigned long long off)
+; asm("llvm.bpf.load.word");
+;
+; int
+; foo(void *buf)
+; {
+; unsigned long long sum = 0;
+;
+; sum += load_word(buf, 100);
+; sum += load_word(buf, 104);
+;
+; if (sum != 0x1ffffffffULL)
+; return ~0U;
+;
+; return 0;
+;}
+
+; Function Attrs: nounwind readonly
+define i32 @foo(i8*) local_unnamed_addr #0 {
+ %2 = tail call i64 @llvm.bpf.load.word(i8* %0, i64 100)
+ %3 = tail call i64 @llvm.bpf.load.word(i8* %0, i64 104)
+ %4 = add i64 %3, %2
+ %5 = icmp ne i64 %4, 8589934591
+; CHECK: r{{[0-9]+}} = 8589934591 ll
+ %6 = sext i1 %5 to i32
+ ret i32 %6
+}
+
+; Function Attrs: nounwind readonly
+declare i64 @llvm.bpf.load.word(i8*, i64) #1
--
2.7.4


Jesper Dangaard Brouer
 

On Thu, 12 Oct 2017 06:21:30 -0400
Jiong Wang <jiong.wang@...> wrote:

We came across an llvm bug when compiling some testcases that 64-bit
immediates are silently truncated into 32-bit and then packed into
BPF_JMP | BPF_K encoding. This caused comparison with wrong value.
I think you send this to the wrong mailing list... this looks like a
patch against the LLVM source code.

Shouldn't you send to: llvm-dev@... ?

This bug looks to be introduced by r308080.
This looks like a very recent change:
https://llvm.org/viewvc/llvm-project?view=revision&revision=308080
Sat Jul 15 05:41:42 2017 UTC (2 months, 4 weeks ago)

(As you are sending this to a user mailing list: xdp-newbies@...)

I want to know if this have made it into a LLVM release? and which release?

As the git-repo[1] replica of LLVM SVN-repo does not git-tag the
releases, I could not answer this question myself via the command:

$ git describe --contains 7c423e0690

[1] http://llvm.org/git/llvm.git

The Select_Ri pattern is
supposed to be lowered into J*_Ri while the latter only support 32-bit
immediate encoding, therefore Select_Ri should have similar immediate
predicate check as what J*_Ri are doing.

Reported-by: Jakub Kicinski <jakub.kicinski@...>
Signed-off-by: Jiong Wang <jiong.wang@...>
---
lib/Target/BPF/BPFISelLowering.cpp | 8 ++++++--
lib/Target/BPF/BPFInstrInfo.td | 2 +-
test/CodeGen/BPF/select_ri.ll | 35 +++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 3 deletions(-)
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer


Jiong Wang
 

On 12/10/2017 12:04, Jesper Dangaard Brouer wrote:
On Thu, 12 Oct 2017 06:21:30 -0400
Jiong Wang <jiong.wang@...> wrote:

We came across an llvm bug when compiling some testcases that 64-bit
immediates are silently truncated into 32-bit and then packed into
BPF_JMP | BPF_K encoding. This caused comparison with wrong value.
I think you send this to the wrong mailing list... this looks like a
patch against the LLVM source code.

Shouldn't you send to: llvm-dev@... ?
Hi Jesper,

I am thinking the users & developers of eBPF llvm are quite focused in
xdp-newbies and iovisor-dev, so if the code is related with user visible
effect, for example bug fix, and if the code change is straightforward
and small, I tend to just send it to these two so related user could be
aware of it in case they are encountering the same problem, and then this
patch could be posted to llvm review site (https://reviews.llvm.org).


This bug looks to be introduced by r308080.
This looks like a very recent change:
https://llvm.org/viewvc/llvm-project?view=revision&revision=308080
Sat Jul 15 05:41:42 2017 UTC (2 months, 4 weeks ago)

(As you are sending this to a user mailing list: xdp-newbies@...)

I want to know if this have made it into a LLVM release? and which release?
This has not been made into any LLVM official release.
The bug may show up if the user is trying to use distributor snapshop release.


As the git-repo[1] replica of LLVM SVN-repo does not git-tag the
releases, I could not answer this question myself via the command:

$ git describe --contains 7c423e0690

[1] http://llvm.org/git/llvm.git

The Select_Ri pattern is
supposed to be lowered into J*_Ri while the latter only support 32-bit
immediate encoding, therefore Select_Ri should have similar immediate
predicate check as what J*_Ri are doing.

Reported-by: Jakub Kicinski <jakub.kicinski@...>
Signed-off-by: Jiong Wang <jiong.wang@...>
---
lib/Target/BPF/BPFISelLowering.cpp | 8 ++++++--
lib/Target/BPF/BPFInstrInfo.td | 2 +-
test/CodeGen/BPF/select_ri.ll | 35 +++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 3 deletions(-)
--

Regards,
Jiong


Yonghong Song
 

Hi, Jiong,

The patch looks good. Thanks for fixing the issue. Will merge soon.

Yonghong

On Thu, Oct 12, 2017 at 5:08 AM, Jiong Wang via iovisor-dev
<iovisor-dev@...> wrote:
On 12/10/2017 12:04, Jesper Dangaard Brouer wrote:

On Thu, 12 Oct 2017 06:21:30 -0400
Jiong Wang <jiong.wang@...> wrote:

We came across an llvm bug when compiling some testcases that 64-bit
immediates are silently truncated into 32-bit and then packed into
BPF_JMP | BPF_K encoding. This caused comparison with wrong value.
I think you send this to the wrong mailing list... this looks like a
patch against the LLVM source code.

Shouldn't you send to: llvm-dev@... ?

Hi Jesper,

I am thinking the users & developers of eBPF llvm are quite focused in
xdp-newbies and iovisor-dev, so if the code is related with user visible
effect, for example bug fix, and if the code change is straightforward
and small, I tend to just send it to these two so related user could be
aware of it in case they are encountering the same problem, and then this
patch could be posted to llvm review site (https://reviews.llvm.org).



This bug looks to be introduced by r308080.
This looks like a very recent change:
https://llvm.org/viewvc/llvm-project?view=revision&revision=308080
Sat Jul 15 05:41:42 2017 UTC (2 months, 4 weeks ago)

(As you are sending this to a user mailing list:
xdp-newbies@...)

I want to know if this have made it into a LLVM release? and which
release?

This has not been made into any LLVM official release.
The bug may show up if the user is trying to use distributor snapshop
release.


As the git-repo[1] replica of LLVM SVN-repo does not git-tag the
releases, I could not answer this question myself via the command:

$ git describe --contains 7c423e0690

[1] http://llvm.org/git/llvm.git

The Select_Ri pattern is
supposed to be lowered into J*_Ri while the latter only support 32-bit
immediate encoding, therefore Select_Ri should have similar immediate
predicate check as what J*_Ri are doing.

Reported-by: Jakub Kicinski <jakub.kicinski@...>
Signed-off-by: Jiong Wang <jiong.wang@...>
---
lib/Target/BPF/BPFISelLowering.cpp | 8 ++++++--
lib/Target/BPF/BPFInstrInfo.td | 2 +-
test/CodeGen/BPF/select_ri.ll | 35
+++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 3 deletions(-)

--

Regards,
Jiong

_______________________________________________
iovisor-dev mailing list
iovisor-dev@...
https://lists.iovisor.org/mailman/listinfo/iovisor-dev