[llvm] 15f2d4f - [AIX] Fixed "comparison of unsigned expression >= 0 is always true" gcc warnings.

Sat Dec 19 11:55:07 PST 2020

On Fri, Dec 18, 2020 at 2:26 PM Hubert Tong <
hubert.reinterpretcast at gmail.com> wrote:

> On Fri, Dec 18, 2020 at 4:13 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Fri, Dec 18, 2020 at 10:06 AM digger lin <digger.llvm at gmail.com>
>> wrote:
>>
>>> added  a new NFC patch
>>> https://reviews.llvm.org/rGd551e40f1cff7a63218f34112bd0dddaa2b12dbb to
>>> address the comment.
>>>
>>
>> Thanks!
>>
>> Though looking at it, the original code seems to have been buggy, rather
>> than just redundant, right? (the condition would never have fired) - the
>> code under the "if" condition hasn't been tested?
>>
> The problem was that the code under the "if" condition always ran (even
> when it shouldn't). When it shouldn't, we get undefined behaviour. Since
> there was a report of a ubsan error, it seems that the "if" condition (as
> fixed) does have testing.
>

Oh, great! Thanks for walking me through it!

>
>
>> (since if it had, it would've shown the code to be unreachable) - Could
>> you/someone add the missing test coverage here?
>>
>>
>>>
>>> On Thu, Dec 17, 2020 at 9:17 PM Hubert Tong <
>>> hubert.reinterpretcast at gmail.com> wrote:
>>>
>>>> On Wed, Dec 16, 2020 at 8:47 PM David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Tue, Dec 15, 2020 at 9:24 AM Hubert Tong <
>>>>> hubert.reinterpretcast at gmail.com> wrote:
>>>>>
>>>>>> On Mon, Dec 14, 2020 at 8:36 PM David Blaikie via llvm-commits <
>>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>>
>>>>>>> Any idea if this is the most suitable fix? Is "Bits" representing
>>>>>>> negative values? If not, perhaps it'd be more appropriate to remove the >=
>>>>>>> 0 comparison, rather than to change the type?
>>>>>>>
>>>>>> For context, this aspect of the implementation will likely be
>>>>>> replaced as further functionality (to implement the encoding form when
>>>>>> vector parameters are present) is implemented. In terms of NFC evolution of
>>>>>> this code, the reparsing loop is not ideal. If the code was not being
>>>>>> changed anyway, it would make sense to store the "next position" alongside
>>>>>> the encoded bitstring.
>>>>>>
>>>>>> As for the signedness of the type: I believe changing the type is
>>>>>> appropriate in this case even if the comparison is modified to avoid the
>>>>>> subtraction.
>>>>>>
>>>>>
>>>>> Appropriate in that you think it's a more suitable type for the value
>>>>> being stored in it, even if it weren't for the warning?
>>>>>
>>>> There's been quite the thread that has not gotten consensus one way or
>>>> the other about preferring `int` (see related:
>>>> https://reviews.llvm.org/D63049). The variable here (aside from
>>>> representing something that is never negative) has no particular reason to
>>>> be `unsigned`. So, using `int` to avoid the wraparound at `0` has its
>>>> advantages. Perhaps I would give the rationale of using `unsigned` more
>>>> weight if this was part of a function interface.
>>>>
>>>>
>>>>>
>>>>>
>>>>>> I do agree that changing the comparison should also be done; however,
>>>>>> the change to the type was the easiest change to eyeball in the context of
>>>>>> responding to the breakage.
>>>>>>
>>>>>
>>>>> Would be good to clean that up, then - could someone make that change
>>>>> (& mention the commit hash here)? (otherwise it'll be hard to track down
>>>>> the longer the code evolves - without the warning to point to it)
>>>>>
>>>> @digger lin <digger.llvm at gmail.com>, can you post this change or (if
>>>> the code is ready) the change that removes the current implementation (in
>>>> order to add functionality)?
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> On Mon, Dec 14, 2020 at 8:09 AM via llvm-commits <
>>>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> Author: diggerlin
>>>>>>>> Date: 2020-12-14T11:08:40-05:00
>>>>>>>> New Revision: 15f2d4f198380762e9fcf6b456d405078b87ae7a
>>>>>>>>
>>>>>>>> URL:
>>>>>>>> https://github.com/llvm/llvm-project/commit/15f2d4f198380762e9fcf6b456d405078b87ae7a
>>>>>>>> DIFF:
>>>>>>>> https://github.com/llvm/llvm-project/commit/15f2d4f198380762e9fcf6b456d405078b87ae7a.diff
>>>>>>>>
>>>>>>>> LOG: [AIX] Fixed "comparison of unsigned expression >= 0 is always
>>>>>>>> true" gcc warnings.
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>>
>>>>>>>> fixed a  Fixed "comparison of unsigned expression >= 0 is always
>>>>>>>> true" gcc warnings.
>>>>>>>> http://lab.llvm.org:8011/#/builders/5/builds/2407/steps/2/logs/stdio
>>>>>>>>
>>>>>>>> the error caused by patch https://reviews.llvm.org/D92398
>>>>>>>>
>>>>>>>> Added:
>>>>>>>>
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>     llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>>>>
>>>>>>>> Removed:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> ################################################################################
>>>>>>>> diff  --git a/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>>>> b/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>>>> index 02a425044c75..d364eb9d3996 100644
>>>>>>>> --- a/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>>>> +++ b/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>>>> @@ -67,7 +67,7 @@ bool PPCFunctionInfo::isLiveInZExt(Register VReg)
>>>>>>>> const {
>>>>>>>>
>>>>>>>>  void PPCFunctionInfo::appendParameterType(ParamType Type) {
>>>>>>>>    uint32_t CopyParamType = ParameterType;
>>>>>>>> -  unsigned Bits = 0;
>>>>>>>> +  int Bits = 0;
>>>>>>>>
>>>>>>>>    // If it is fixed type, we only need to increase the
>>>>>>>> FixedParamNum, for
>>>>>>>>    // the bit encode of fixed type is bit of zero, we do not need
>>>>>>>> to change the
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> llvm-commits mailing list
>>>>>>>> llvm-commits at lists.llvm.org
>>>>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at lists.llvm.org
>>>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201219/980a126d/attachment.html>