[llvm-dev] 5.0.1-rc2 has been tagged
Y Song via llvm-dev
llvm-dev at lists.llvm.org
Mon Dec 4 09:15:51 PST 2017
Hi, Tom,
Sorry about my rushness about going ahead to get a BPF patch in as
this is my first time to push a patch to a release branch, and I am not aware of
proper process. I promise to follow proper procedures from now on.
See my other comments below.
On Mon, Dec 4, 2017 at 6:53 AM, Tom Stellard <tstellar at redhat.com> wrote:
> On 12/02/2017 10:44 PM, Y Song wrote:
>> Hi, Tom,
>>
>> Considering the severity of this bug, I would like to go ahead to push
>> the fix into release_50 branch. The fix has been tested in the trunk and by
>> various people as well and I will also make sure all BPF tests passed
>> before the push.
>>
>
> Hi Yonghong,
>
> Thank you for helping out with LLVM release testing.
Yes, in the future, I do plan to test BPF backend regularly for release branch.
>
> Patches need to be approved by the release manager and code owner before they
> can be committed to the release branch.
>
> Take a look at http://llvm.org/docs/HowToReleaseLLVM.html#merge-requests
> for the process of filling a merge request.
Sorry about this as I searched and did not find this document earlier.
Now I know it and will definitely follow it from now on.
>
> This late in the process we usually only accept critical bugs or regression
> fixes.
Understood.
>
> Can you give me a little more information about this particular bug?
> What is the impact to users and how likely are they to encounter this
> issue?
This cilium pull request (https://github.com/cilium/cilium/pull/2162)
provided some information and discussion.
The symptom bug looks like below:
/* here x is a variable */
if (x == #const) /* the same for != */
...
Wrong code will be generated if #const is > 0x7FFFFFFFF.
For example,
if (x == 0xffff0000)
...
will generate wrong code
and
if (x != 0xffff00000000ULL)
will generate wrong code as well.
The bug is pretty nasty and if it hits the user, it is very hard for
them to debug and workaround unless they know compiler internals.
And the chance some people hits this bug is pretty high.
The bug was introduced in 5.0 and was fixed in trunk:
======
commit e53750e1e086f4e234f52041072e688abd79e22b
Author: Yonghong Song <yhs at fb.com>
Date: Mon Oct 16 04:14:53 2017 +0000
bpf: fix bug on silently truncating 64-bit immediate
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 at netronome.com>
Signed-off-by: Jiong Wang <jiong.wang at netronome.com>
Reviewed-by: Yonghong Song <yhs at fb.com>
git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@315889
91177308-0d34-0410-b5e6-96231b3b80d8
======
Fixing the issue in 5.0.1 will make user life easier for those users
using linux-default-installed
5.x llvm/clang compilers, e.g., in latest ubuntu and fedora distro.
Thanks!
Yonghong
>
> Thanks,
> Tom
>
>
>> Thanks!
>>
>> Yonghong
>>
>> On Fri, Dec 1, 2017 at 10:18 AM, Y Song <ys114321 at gmail.com> wrote:
>>> Hi, Tom,
>>>
>>> I have a BPF backend bug which is pretty noxious which is introduced
>>> in 5.0 and fixed in 6.0 trunk.
>>> The detailed of the backport commit is at the end of this email, which
>>> is slightly different from
>>> 6.0 fix to adjust the asm output format. Is there a way for this bug
>>> fix into 5.0.1 release and if yes,
>>> what is the process?
>>>
>>> I have applied the commit below to latest release_50 branch in my
>>> local repo and run through all BPF tests
>>> and they are fine.
>>>
>>> Thanks!
>>>
>>> Yonghong
>>>
>>> ===== Commit to backport Details =====
>>> commit a115edb82180f0c94d692c4abfd631984ada156b
>>> Author: Yonghong Song <yhs at fb.com>
>>> Date: Mon Oct 16 04:14:53 2017 +0000
>>>
>>> bpf: fix bug on silently truncating 64-bit immediate
>>>
>>> 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 (llvm 5.0). 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.
>>>
>>> The bug is fixed by
>>> git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@315889
>>> 91177308-0d34-0410-b5e6-96231b3b80d8
>>> in llvm 6.0.
>>>
>>> This patch is largely the same as the fix in llvm 6.0 except
>>> one minor adjustment ("; CHECK: r{{[0-9]+}} = 8589934591 ll" in
>>> the original commit
>>> to "; CHECK: r{{[0-9]+}} = 8589934591ll" in this patch) for the test case.
>>>
>>> Reported-by: John Fastabend <john.fastabend at gmail.com>
>>> Reported-by: Jakub Kicinski <jakub.kicinski at netronome.com>
>>> Signed-off-by: Jiong Wang <jiong.wang at netronome.com>
>>> Reviewed-by: Yonghong Song <yhs at fb.com>
>>>
>>> diff --git a/lib/Target/BPF/BPFISelLowering.cpp
>>> b/lib/Target/BPF/BPFISelLowering.cpp
>>> index 81b0aa7..5740b49 100644
>>> --- a/lib/Target/BPF/BPFISelLowering.cpp
>>> +++ b/lib/Target/BPF/BPFISelLowering.cpp
>>> @@ -578,11 +578,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 f683578..56f0f9c 100644
>>> --- a/lib/Target/BPF/BPFInstrInfo.td
>>> +++ b/lib/Target/BPF/BPFInstrInfo.td
>>> @@ -464,7 +464,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 c4ac376..3610d40 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]+}} = 8589934591ll
>>> + %6 = sext i1 %5 to i32
>>> + ret i32 %6
>>> +}
>>> +
>>> +; Function Attrs: nounwind readonly
>>> +declare i64 @llvm.bpf.load.word(i8*, i64) #1
>>>
>>> On Wed, Nov 29, 2017 at 4:19 PM, Tom Stellard via llvm-dev
>>> <llvm-dev at lists.llvm.org> wrote:
>>>> Hi,
>>>>
>>>> I've tagged the 5.0.1-rc2 release, go ahead and start testing and report
>>>> your results.
>>>>
>>>> -Tom
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
More information about the llvm-dev
mailing list