compiling stops at od compare

compiling stops at od compare

[Willem Jan Withagen]

When trying to compile on CentOS 7, gcc = 4.8.3

os/bluestore/BlueFS.cc: In member function ‘int
BlueFS::_read(BlueFS::FileReader*, BlueFS::FileReaderBuffer*, uint64_t,
size_t, ceph::bufferlist*, char*)’:
os/bluestore/BlueFS.cc:731:31: warning: comparison between signed and
unsigned integer expressions [-Wsign-compare]
     int r = MIN((int)len, left);

This MIN is used to determine the amount of buffer that is still left
to be filed.
And here len and left are both size_t..., suggesting that both cannot be
negative. So either both need to be promoted/cast, or neither.

The cast (int)len suggests that len could be negative.
The part where that could happen is at line 750:

   off += r;
    len -= r;
    ret += r;

So there the loop exit needs len to be exactly equal to r. Even if the
loop specifies while(len>0). if len gets "negative" it grows into
something rather big.

Now if len never gets negative then it also does not need to get cast to
int. If it does, then in the unsigned case it will always be larger than
left.

So bottomline is that the cast serves no purpose?
Removing it fixes compilation.

--WjW

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: compiling stops at od compare

[Matt Benjamin]

Should we use std::min here (that might require a cast, iirc)?

Matt

----- Original Message -----
> From: "Willem Jan Withagen" <wjw@digiware.nl>
> To: ceph-devel@vger.kernel.org
> Sent: Saturday, January 30, 2016 8:22:07 AM
> Subject: compiling stops at od compare
> 
> When trying to compile on CentOS 7, gcc = 4.8.3
> 
> os/bluestore/BlueFS.cc: In member function ‘int
> BlueFS::_read(BlueFS::FileReader*, BlueFS::FileReaderBuffer*, uint64_t,
> size_t, ceph::bufferlist*, char*)’:
> os/bluestore/BlueFS.cc:731:31: warning: comparison between signed and
> unsigned integer expressions [-Wsign-compare]
>      int r = MIN((int)len, left);
> 
> This MIN is used to determine the amount of buffer that is still left
> to be filed.
> And here len and left are both size_t..., suggesting that both cannot be
> negative. So either both need to be promoted/cast, or neither.
> 
> The cast (int)len suggests that len could be negative.
> The part where that could happen is at line 750:
> 
>    off += r;
>     len -= r;
>     ret += r;
> 
> So there the loop exit needs len to be exactly equal to r. Even if the
> loop specifies while(len>0). if len gets "negative" it grows into
> something rather big.
> 
> Now if len never gets negative then it also does not need to get cast to
> int. If it does, then in the unsigned case it will always be larger than
> left.
> 
> So bottomline is that the cast serves no purpose?
> Removing it fixes compilation.
> 
> --WjW
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
-- 
Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-707-0660
fax.  734-769-8938
cel.  734-216-5309
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: compiling stops at od compare

[Willem Jan Withagen]

On 30-1-2016 23:57, Matt Benjamin wrote:
> Should we use std::min here (that might require a cast, iirc)?

Well the most important issue I have:
	while(len>0)
where len is of type size_t has only exactly one chance to be true, aka
len = 0. negative numbers do not exist.

So casting it to int is a bad(tm) thing.

But as I haven't designed the code, i can only react to the compiler
error and analyze it. And then my conclusion is that the cast can only
increase the chance on an error. And thus the compiler is correct in
triggering.

--WjW

> 
> Matt
> 
> ----- Original Message -----
>> From: "Willem Jan Withagen" <wjw@digiware.nl>
>> To: ceph-devel@vger.kernel.org
>> Sent: Saturday, January 30, 2016 8:22:07 AM
>> Subject: compiling stops at od compare
>>
>> When trying to compile on CentOS 7, gcc = 4.8.3
>>
>> os/bluestore/BlueFS.cc: In member function ‘int
>> BlueFS::_read(BlueFS::FileReader*, BlueFS::FileReaderBuffer*, uint64_t,
>> size_t, ceph::bufferlist*, char*)’:
>> os/bluestore/BlueFS.cc:731:31: warning: comparison between signed and
>> unsigned integer expressions [-Wsign-compare]
>>      int r = MIN((int)len, left);
>>
>> This MIN is used to determine the amount of buffer that is still left
>> to be filed.
>> And here len and left are both size_t..., suggesting that both cannot be
>> negative. So either both need to be promoted/cast, or neither.
>>
>> The cast (int)len suggests that len could be negative.
>> The part where that could happen is at line 750:
>>
>>    off += r;
>>     len -= r;
>>     ret += r;
>>
>> So there the loop exit needs len to be exactly equal to r. Even if the
>> loop specifies while(len>0). if len gets "negative" it grows into
>> something rather big.
>>
>> Now if len never gets negative then it also does not need to get cast to
>> int. If it does, then in the unsigned case it will always be larger than
>> left.
>>
>> So bottomline is that the cast serves no purpose?
>> Removing it fixes compilation.
>>
>> --WjW
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] Re: compiling stops at od compare

[Willem Jan Withagen]

On 31-1-2016 00:13, Willem Jan Withagen wrote:
> On 30-1-2016 23:57, Matt Benjamin wrote:
>> Should we use std::min here (that might require a cast, iirc)?
> 
> Well the most important issue I have:
> 	while(len>0)
> where len is of type size_t has only exactly one chance to be true, aka
> len = 0. negative numbers do not exist.
> 
> So casting it to int is a bad(tm) thing.
> 
> But as I haven't designed the code, i can only react to the compiler
> error and analyze it. And then my conclusion is that the cast can only
> increase the chance on an error. And thus the compiler is correct in
> triggering.

Sorry,

I did not answer your question.

If using std::min requires a cast, then it will fall in the same pitfall
as the current code does. if there is a std::min version that does not
autocast/promote size_t to int it might work as well.
Using MIN does "the right thing", without the cast.

diff --git a/src/os/bluestore/BlueFS.cc b/src/os/bluestore/BlueFS.cc
index 1bad6a2..13555c9 100644
--- a/src/os/bluestore/BlueFS.cc
+++ b/src/os/bluestore/BlueFS.cc
@@ -728,7 +728,7 @@ int BlueFS::_read(
     left = buf->get_buf_remaining(off);
     dout(20) << __func__ << " left " << left << " len " << len << dendl;

-    int r = MIN((int)len, left);
+    int r = MIN(len, left);
     if (outbl) {
       bufferlist t;
       t.substr_of(buf->bl, off - buf->bl_off, r);

This does the job on Centos 7 for me....

--WjW

>> ----- Original Message -----
>>> From: "Willem Jan Withagen" <wjw@digiware.nl>
>>> To: ceph-devel@vger.kernel.org
>>> Sent: Saturday, January 30, 2016 8:22:07 AM
>>> Subject: compiling stops at od compare
>>>
>>> When trying to compile on CentOS 7, gcc = 4.8.3
>>>
>>> os/bluestore/BlueFS.cc: In member function ‘int
>>> BlueFS::_read(BlueFS::FileReader*, BlueFS::FileReaderBuffer*, uint64_t,
>>> size_t, ceph::bufferlist*, char*)’:
>>> os/bluestore/BlueFS.cc:731:31: warning: comparison between signed and
>>> unsigned integer expressions [-Wsign-compare]
>>>      int r = MIN((int)len, left);
>>>
>>> This MIN is used to determine the amount of buffer that is still left
>>> to be filed.
>>> And here len and left are both size_t..., suggesting that both cannot be
>>> negative. So either both need to be promoted/cast, or neither.
>>>
>>> The cast (int)len suggests that len could be negative.
>>> The part where that could happen is at line 750:
>>>
>>>    off += r;
>>>     len -= r;
>>>     ret += r;
>>>
>>> So there the loop exit needs len to be exactly equal to r. Even if the
>>> loop specifies while(len>0). if len gets "negative" it grows into
>>> something rather big.
>>>
>>> Now if len never gets negative then it also does not need to get cast to
>>> int. If it does, then in the unsigned case it will always be larger than
>>> left.
>>>
>>> So bottomline is that the cast serves no purpose?
>>> Removing it fixes compilation.
>>>
>>> --WjW
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html