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

[Hubert Tong via llvm-commits]

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.


> (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/20201218/55e97db9/attachment.html>