Обсуждение: Win64 warnings about size_t
I have adapted the win64 patches a bit, and now have a working build. As in it runs the regression tests fine. However, I have well over a thousand warnings of the type: conversion from 'size_t' to 'int', possible loss of data My first 5-6 checks of where these happen are all cases where we assign the result of strlen() something to an int, or call a function taking an int as parameter with the result of strlen() in there. strlen() returns size_t, which AFAICS is per the standard and not even a Microsoft-specific idea. size_t is 8-bit - but it appears to be 8-bit on Linux as well, when in 64-bit mode. So I don't really see what win64 does differently in this case, but perhaps I've been looking at this code too long? Or is it simply that MSVC warns about this and GCC doesn't, and I shuld disbale the warning? A couple of points for reference: 1>.\src\backend\utils\adt\varlena.c(84) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data 1>.\src\backend\utils\adt\varlena.c(158) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data 1>.\src\backend\utils\adt\varlena.c(158) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data 1>.\src\backend\utils\adt\varlena.c(201) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data 1>.\src\backend\utils\adt\varlena.c(203) : warning C4267: 'function' : conversion from 'size_t' to 'unsigned int', possible loss of data 1>.\src\backend\utils\adt\varlena.c(488) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data 1>.\src\backend\utils\adt\varlena.c(2156) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data 1>.\src\backend\utils\adt\varlena.c(3131) : warning C4267: '=' : conversion from 'size_t' to 'int32', possible loss of data 1>.\src\backend\utils\adt\varlena.c(3136) : warning C4267: '=' : conversion from 'size_t' to 'int32', possible loss of data -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> My first 5-6 checks of where these happen are all cases where we
> assign the result of strlen() something to an int, or call a function
> taking an int as parameter with the result of strlen() in there.
Yeah. Getting rid of all those cases is impractical I think, and
pointless anyway --- we have limitations in palloc and Datum
representation that ensure we'll never be dealing with strings (or other
values) larger than 1GB.
> strlen() returns size_t, which AFAICS is per the standard and not even
> a Microsoft-specific idea.
Correct.
> So I don't really see what win64 does differently in this case, but
> perhaps I've been looking at this code too long? Or is it simply that
> MSVC warns about this and GCC doesn't, and I shuld disbale the
> warning?
I think MSVC is just complaining about something gcc doesn't. If you
can disable this specific warning it'd be a good plan.
regards, tom lane
On Fri, Jan 1, 2010 at 18:47, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> My first 5-6 checks of where these happen are all cases where we >> assign the result of strlen() something to an int, or call a function >> taking an int as parameter with the result of strlen() in there. > > Yeah. Getting rid of all those cases is impractical I think, and > pointless anyway --- we have limitations in palloc and Datum > representation that ensure we'll never be dealing with strings (or other > values) larger than 1GB. Ok. >> strlen() returns size_t, which AFAICS is per the standard and not even >> a Microsoft-specific idea. > > Correct. > >> So I don't really see what win64 does differently in this case, but >> perhaps I've been looking at this code too long? Or is it simply that >> MSVC warns about this and GCC doesn't, and I shuld disbale the >> warning? > > I think MSVC is just complaining about something gcc doesn't. If you > can disable this specific warning it'd be a good plan. Yeah, that should be doable. According to http://msdn.microsoft.com/en-us/library/6kck0s93(VS.80).aspx this warning is only about size_t, and not about the issue in general, so I can just disable C4267 and they should go away. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes: > On Fri, Jan 1, 2010 at 18:47, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I think MSVC is just complaining about something gcc doesn't. �If you >> can disable this specific warning it'd be a good plan. > Yeah, that should be doable. According to > http://msdn.microsoft.com/en-us/library/6kck0s93(VS.80).aspx this > warning is only about size_t, and not about the issue in general, so I > can just disable C4267 and they should go away. Sounds good. The case we do need to look for is any remaining casts between pointer and long. If that's a different warning number then we'll be in good shape. regards, tom lane
On Fri, Jan 1, 2010 at 18:59, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Fri, Jan 1, 2010 at 18:47, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I think MSVC is just complaining about something gcc doesn't. If you >>> can disable this specific warning it'd be a good plan. > >> Yeah, that should be doable. According to >> http://msdn.microsoft.com/en-us/library/6kck0s93(VS.80).aspx this >> warning is only about size_t, and not about the issue in general, so I >> can just disable C4267 and they should go away. > > Sounds good. The case we do need to look for is any remaining casts > between pointer and long. If that's a different warning number then > we'll be in good shape. I don't see any such warnings at all. I do see this however: .\src\backend\utils\mmgr\aset.c(701): warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) .\src\backend\utils\mmgr\aset.c(705): warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) Perhaps we need some casting there? Other than that, I see a few more API related warnings that are not related directly to 32/64-bit. I'l be working on those. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On fre, 2010-01-01 at 20:01 +0100, Magnus Hagander wrote: > .\src\backend\utils\mmgr\aset.c(701): warning C4334: '<<' : result > of 32-bit shift implicitly converted to 64 bits (was 64-bit shift > intended?) > .\src\backend\utils\mmgr\aset.c(705): warning C4334: '<<' : result > of 32-bit shift implicitly converted to 64 bits (was 64-bit shift > intended?) > > Perhaps we need some casting there? This shouldn't be a problem for the same reason that casting size_t to int is not a problem in the PostgreSQL backend code, but perhaps writing 1L << ... would fix it.
On Fri, Jan 1, 2010 at 20:12, Peter Eisentraut <peter_e@gmx.net> wrote: > On fre, 2010-01-01 at 20:01 +0100, Magnus Hagander wrote: >> .\src\backend\utils\mmgr\aset.c(701): warning C4334: '<<' : result >> of 32-bit shift implicitly converted to 64 bits (was 64-bit shift >> intended?) >> .\src\backend\utils\mmgr\aset.c(705): warning C4334: '<<' : result >> of 32-bit shift implicitly converted to 64 bits (was 64-bit shift >> intended?) >> >> Perhaps we need some casting there? > > This shouldn't be a problem for the same reason that casting size_t to > int is not a problem in the PostgreSQL backend code, but perhaps writing > 1L << ... would fix it. 1L didn't fix it. 1LL did, however. ISTM that this is a warning we don't want to disable, so assuming that should be safe on other platforms (it passes on my 32-bit linux without warnings or anything), I'd vote for putting it in there. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Fri, Jan 1, 2010 at 20:12, Peter Eisentraut <peter_e@gmx.net> wrote:
>> This shouldn't be a problem for the same reason that casting size_t to
>> int is not a problem in the PostgreSQL backend code, but perhaps writing
>> 1L << ... would fix it.
> 1L didn't fix it. 1LL did, however.
... and would break things on many many other platforms.
Use "(Size) 1" instead of "1" if you really want to suppress this.
regards, tom lane
On Fri, Jan 1, 2010 at 20:33, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Fri, Jan 1, 2010 at 20:12, Peter Eisentraut <peter_e@gmx.net> wrote: >>> This shouldn't be a problem for the same reason that casting size_t to >>> int is not a problem in the PostgreSQL backend code, but perhaps writing >>> 1L << ... would fix it. > >> 1L didn't fix it. 1LL did, however. > > ... and would break things on many many other platforms. That's what I was afraid of. > Use "(Size) 1" instead of "1" if you really want to suppress this. That fixes it as well. Will apply a patch to that effect. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander wrote: > I have adapted the win64 patches a bit, and now have a working build. > As in it runs the regression tests fine. However, I have well over a > thousand warnings of the type: > conversion from 'size_t' to 'int', possible loss of data > > My first 5-6 checks of where these happen are all cases where we > assign the result of strlen() something to an int, or call a function > taking an int as parameter with the result of strlen() in there. > > strlen() returns size_t, which AFAICS is per the standard and not even > a Microsoft-specific idea. size_t is 8-bit - but it appears to be > 8-bit on Linux as well, when in 64-bit mode. Uh, you mean size_t is 8 _bytes_ on Win64? That would make sense. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sat, Jan 2, 2010 at 03:13, Bruce Momjian <bruce@momjian.us> wrote: > Magnus Hagander wrote: >> I have adapted the win64 patches a bit, and now have a working build. >> As in it runs the regression tests fine. However, I have well over a >> thousand warnings of the type: >> conversion from 'size_t' to 'int', possible loss of data >> >> My first 5-6 checks of where these happen are all cases where we >> assign the result of strlen() something to an int, or call a function >> taking an int as parameter with the result of strlen() in there. >> >> strlen() returns size_t, which AFAICS is per the standard and not even >> a Microsoft-specific idea. size_t is 8-bit - but it appears to be >> 8-bit on Linux as well, when in 64-bit mode. > > Uh, you mean size_t is 8 _bytes_ on Win64? That would make sense. Yes, 8 bytes, 64 bit. Of course :-) Sorry. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander wrote: > On Sat, Jan 2, 2010 at 03:13, Bruce Momjian <bruce@momjian.us> wrote: > > Magnus Hagander wrote: > >> I have adapted the win64 patches a bit, and now have a working build. > >> As in it runs the regression tests fine. However, I have well over a > >> thousand warnings of the type: > >> conversion from 'size_t' to 'int', possible loss of data > >> > >> My first 5-6 checks of where these happen are all cases where we > >> assign the result of strlen() something to an int, or call a function > >> taking an int as parameter with the result of strlen() in there. > >> > >> strlen() returns size_t, which AFAICS is per the standard and not even > >> a Microsoft-specific idea. size_t is 8-bit - but it appears to be > >> 8-bit on Linux as well, when in 64-bit mode. > > > > Uh, you mean size_t is 8 _bytes_ on Win64? ?That would make sense. > > Yes, 8 bytes, 64 bit. Of course :-) Sorry. If we are storing potentially 64-bit values in 32-bit variables and we know the value is going to fit, it would be nice if we could document this some way, perhaps with some typedef data type. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +