[llvm-dev] 5.0.1-rc2 has been tagged
Tom Stellard via llvm-dev
llvm-dev at lists.llvm.org
Tue Dec 5 08:47:59 PST 2017
On 12/04/2017 09:15 AM, Y Song wrote:
> 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.
>
Don't worry about it, I know the documentation can be hard to track down sometimes.
It does sound like this is pretty critical, so I've gone ahead and created
a merge request for this: https://bugs.llvm.org/show_bug.cgi?id=35532 so
we can get Alexey's opinion.
I think for now it is OK to leave the patch in the release branch if
for some reason Alexey objects we can just revert it
Thanks,
Tom
> 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