Board index » cppbuilder » suspicious pointer arithmetic

suspicious pointer arithmetic


2005-12-01 11:17:47 PM
cppbuilder96
Borland C++ Builder 6 compiler gives a warning on "suspicious pointer
arithmetic" (W8072) on the following code:
typedef unsigned char ubyte;
typedef __int64 int64;
bool bit_is_set(ubyte bits[], int64 bit)
{
int64 pa = bit / CHAR_BIT;
int64 pb = bit % CHAR_BIT;
if(bits[pa] & (1 << pb)) // compiler warns here
return true;
else
return false;
}
the bits[] is an array of bytes that must serve as a bitmap.
should i ignore the warning or bits[] array indeed cannot be addressed
by int64 value?
Visual C++ 7.0 compiler is silent about this.
Thanks,
Muzaffar
 
 

Re:suspicious pointer arithmetic

Muzaffar Mahkamov < XXXX@XXXXX.COM >wrote:
Quote
Borland C++ Builder 6 compiler gives a warning on "suspicious pointer
arithmetic" (W8072) on the following code:

typedef unsigned char ubyte;
typedef __int64 int64;

bool bit_is_set(ubyte bits[], int64 bit)
{
int64 pa = bit / CHAR_BIT;
int64 pb = bit % CHAR_BIT;

if(bits[pa] & (1 << pb)) // compiler warns here
return true;
else
return false;
}

the bits[] is an array of bytes that must serve as a bitmap.

should i ignore the warning or bits[] array indeed cannot be addressed
by int64 value?
Well, since BCB6 isn't a 64-bit compiler - attempting to use 64-bit
addressing into an array is something to be warned about.
Basically, the expression 'bits[pa]' is equivalent to '*(bits + pa)'.
You are adding a 64-bit offset to a 32-bit pointer. This is a sensible
warning - it _is_ suspicious. To avoid it, make pa an unsigned long
(suitably casting its initialisation).
The compiler might also get upset at the fact that in '(1 << pb)' would,
for 99.999999+% of all possible values of pb, result in something that
wouldn't fit in a byte. You might also wish to limit the size of pb to
something smaller. A ubyte would do.
Quote
Visual C++ 7.0 compiler is silent about this.
Not all compilers agree on what should be warned about. A warning is a
warning - the compiler saying "This is legal, but it might not do what
you expected".
Alan Bellingham
--
ACCU Conference 2006 - 19-22 April, Randolph Hotel, Oxford, UK
 

Re:suspicious pointer arithmetic

Andr?Taffarello wrote:
Quote
BCB is probably warning you about the "&"
It thinks you're misplacing a "&&" with a single "&"

Also tried this:
bool bit_is_set(ubyte bits[], int64 bit)
{
int64 pa = bit / CHAR_BIT;
int64 pb = bit % CHAR_BIT;
ubyte b = bits[pa] // compiler warns here
if(bits[pa] & (1 << pb)) // compiler warns here
return true;
else
return false;
}
According to Borland C++ Builder help this warning is about array
indexing. Quote from the help:
Quote
This message indicates an unintended side effect to the pointer arithmetic (or array indexing) found in an expression.

Example:

#pragma warn +spa

int array[10];

int foo(__int64 index)
{
return array[index];
}


The value of index is 64 bits wide while the address of array is only 32 bits wide.
So I'm unsure that my code will work as intended.
Muzaffar
 

{smallsort}

Re:suspicious pointer arithmetic

Muzaffar Mahkamov < XXXX@XXXXX.COM >writes:
Quote
Borland C++ Builder 6 compiler gives a warning on "suspicious pointer
arithmetic" (W8072) on the following code:

typedef unsigned char ubyte;
typedef __int64 int64;
bool bit_is_set(ubyte bits[], int64 bit)
{
int64 pa = bit / CHAR_BIT;
int64 pb = bit % CHAR_BIT;

if(bits[pa] & (1 << pb)) // compiler warns here
should i ignore the warning or bits[] array indeed cannot be addressed
by int64 value?
This kind of warning is the compiler looking out for you, and whether
it warns or not does not indicate an actual error on your code. It
signifies something the compiler deems error-prone.
I don't like to ignore warnings, and if I can re-arrange the code to
silence it, then I will (provided it doesn't hurt performance.)
Since dereferencing an array is no different than taking a pointer (p)
and adding (n) to it, and dereferencing that, the compiler may be
complaining that you're adding a 64-bit number to a pointer
(indirectly).
Sure, if you know the result is in range it's not a problem, but there
is an obvious potential problem if you only consider the types, since
a pointer is smaller than 64-bits, there is room for pointer
overflow, etc.
Do you really need a 64-bit number to index into the array? If you
can make "pa" be a normal int you'll probably avoid the warning.
What are you gaining by using a 64-bit integer anyway? I don't see
any need to be able to represent more than 4 billion bits, since the
memory requirements are unreasonable for most applications. And since
64-bits are not native to the platform, it may be hurting performace
rather than helping. Consider benchmarking...
--
Chris (TeamB);
 

Re:suspicious pointer arithmetic

Muzaffar Mahkamov wrote:
Quote
should i ignore the warning or bits[] array indeed cannot be
addressed by int64 value?
I would imagine it can't be. Win32 only supports 32 bit memory
addresses (up to 2GB). I don't know why VC wouldn't issue a warning but
that's up to the compiler. What will likely happen is that the top 32
bits will be discarded when addressing the array.
As for the warning - you can ignore it but you shouldn't. Best thing
would be to cast it to an unsigned __int32. That way you are telling
the compiler and anyone reading your code that you know what you're
doing. Hopefully doing that would also cause you to add bounds checking
logic somewhere since dividing by 8 still doesn't guarantee that the
index will be valid.
Presumably a 64 bit compiler for Win64 wouldn't have a problem with
that code but I'd like to see a machine that had enough storage on it
to back that array :D
--
Andrue Cope [TeamB]
[Bicester, Uk]
info.borland.com/newsgroups/guide.html
 

Re:suspicious pointer arithmetic

Alan Bellingham wrote:
Quote

Well, since BCB6 isn't a 64-bit compiler - attempting to use 64-bit
addressing into an array is something to be warned about.

Basically, the expression 'bits[pa]' is equivalent to '*(bits + pa)'.
You are adding a 64-bit offset to a 32-bit pointer. This is a sensible
warning - it _is_ suspicious. To avoid it, make pa an unsigned long
(suitably casting its initialisation).


Alan Bellingham
Thank you, it's worked fine (no warnings). But afaik, 'unsigned long' is
also 64-bits wide. What's the difference?
 

Re:suspicious pointer arithmetic

Muzaffar Mahkamov wrote:
Quote
Thank you, it's worked fine (no warnings). But afaik, 'unsigned long'
is also 64-bits wide.
No, it's 32 bits.
'long' means the same as 'int' for BCB6.
--
Andrue Cope [TeamB]
[Bicester, Uk]
info.borland.com/newsgroups/guide.html
 

Re:suspicious pointer arithmetic

Chris Uzdavinis (TeamB) wrote:
Quote
I don't see any need to be able to represent more than 4 billion bits
Tracking the usage of storage media in excess of 2TB springs to mind
for some reason(*).
Luckily our bitmap already swaps out to a file so I know we're covered.
(*)This week I'm extending our library so it can handle devices with
2^64 blocks :)
--
Andrue Cope [TeamB]
[Bicester, Uk]
info.borland.com/newsgroups/guide.html
 

Re:suspicious pointer arithmetic

Muzaffar Mahkamov wrote:
Quote
>The value of index is 64 bits wide while the address of array is
>only 32 bits wide.

So I'm unsure that my code will work as intended.
It will with BCB6 up to a maximum bit size of 2^35. That's actually a
none-issue because long before then you'll have hit memory constraints.
If your array is going to grow beyond 1MB I'd suggest paging it out to
disk. It's not a particularly difficult project. Write it as a
self-contained class and it's actually quite good fun. More if you
choose to try and optimise caching.
--
Andrue Cope [TeamB]
[Bicester, Uk]
info.borland.com/newsgroups/guide.html
 

Re:suspicious pointer arithmetic

Chris Uzdavinis (TeamB) wrote:
Quote
Muzaffar Mahkamov < XXXX@XXXXX.COM >writes:

Do you really need a 64-bit number to index into the array? If you
can make "pa" be a normal int you'll probably avoid the warning.

What are you gaining by using a 64-bit integer anyway? I don't see
any need to be able to represent more than 4 billion bits, since the
memory requirements are unreasonable for most applications. And since
64-bits are not native to the platform, it may be hurting performace
rather than helping. Consider benchmarking...

I think you're right. In practice, the application won't have or need
that much data in the array.
The array is used as a bitmap to address blocks in the storage (as in
file systems). The reason I chose int64 was to safely translate 64-bit
data stream positions in the storage (larger than 4GB) to block
addresses. As I see now, it's technically impossible to fit that bitmap
in 32-bit systems even if it grows that much (but in practice it won't) :)
 

Re:suspicious pointer arithmetic

Muzaffar Mahkamov wrote:
Quote
I think you're right. In practice, the application won't have or need
that much data in the array.

The array is used as a bitmap to address blocks in the storage (as in
file systems).
Lol. Been there. Done that :)
class TOVogonBitmapHandler
{
QWORD MySizeOfBitMapInBytes,
MyLastByteIndex,
MyCacheStart,
MyCacheEnd,
MyCacheLastHit,
MyCacheSize;
bool MyCacheDirtyF;
WORD MyBulkSize;
boost::shared_array<BYTE>MyCache,
MyBulkBuffer;
BYTE MyLastDataByte;
TOVS MyTemporaryDirectory,
MyOutputFilePath;
bool MyDeleteOutputFileOnExitF;
std::auto_ptr<TOFSDrv>
MyFileHandle;
long _fastcall flushCachedData( bool ClearCacheF=false ),
_fastcall getDataByte( QWORD ByteIndex,
BYTE & DataByte ),
_fastcall putDataByte( QWORD ByteIndex,
BYTE DataByte ),
_fastcall setBitRangeStatus( QWORD RangeStart,
QWORD RangeTotal,
bool BitStatus,
QWORD *
NumberOfBitsChanged=NULL ),
_fastcall setBulkRangeStatus( QWORD ByteOffset,
QWORD ByteCount,
BYTE StatusValue ),
_fastcall populateCache( QWORD ByteOffset ),
_fastcall partialPopulateCache( QWORD First,
QWORD Last );
public:
TOVogonBitmapHandler( QWORD NumberOfBitsToHandle,
TOVS const & TemporaryDirectory,
long & ErrorCode,
bool PreInitialiseValueF=false,
TOVS const &
SpecificWorkingFilePathOrNULL=TOVS(),
// If not specified a temporary file is
created.
// If specified the file is not deleted when
the class exits
bool ReinitialiseExistingF=true
);
virtual ~TOVogonBitmapHandler();
long WriteCurrentStateToAFile( TOVS const & FilePath,
TCBCopyProgress
ProgressCallBack=NULL );
long _fastcall GetBitStatus( QWORD BitNumber,
bool & BitStatusF );
long _fastcall SetBitRangeStatus( QWORD RangeStart,
QWORD RangeTotal,
bool BitStatus,
QWORD * NumberOfBitsChanged=NULL
);
QWORD SetCacheSize( QWORD NumberOfBitsToCache );
void MaximiseCacheSize(),
MinimiseCacheSize(),
DefaultCacheSize();
};
..unfortunately the .CPP has to remain hidden :)
--
Andrue Cope [TeamB]
[Bicester, Uk]
info.borland.com/newsgroups/guide.html
 

Re:suspicious pointer arithmetic

Muzaffar Mahkamov < XXXX@XXXXX.COM >writes:
Quote
Alan Bellingham wrote:
>Well, since BCB6 isn't a 64-bit compiler - attempting to use 64-bit
>addressing into an array is something to be warned about.
>Basically, the expression 'bits[pa]' is equivalent to '*(bits + pa)'.
>You are adding a 64-bit offset to a 32-bit pointer. This is a sensible
>warning - it _is_ suspicious. To avoid it, make pa an unsigned long
>(suitably casting its initialisation).
>Alan Bellingham

Thank you, it's worked fine (no warnings). But afaik, 'unsigned long'
is also 64-bits wide. What's the difference?
That's not necessarily true. On a 64-bit machine it may be true, but
the C++ language does not define how big a "short", "int", or "long"
actually is, just that they're at least as big as the previous. That
is, the order is
sizeof(char) <= sizeof(short) <= sizeof(int) <= sizeof(long)
So it is possible that reult of sizeof(long) is just 1, the same value
that you will always get for a char. On Crays, I believe, this
actually happens. Chars are 64 bits, and so are shorts, ints, and
longs. (And sizeof(char) is still 1 regardless of how many bits are
used to represent it, because that's by definition.)
--
Chris (TeamB);
 

Re:suspicious pointer arithmetic

BCB is probably warning you about the "&"
It thinks you're misplacing a "&&" with a single "&"
"Muzaffar Mahkamov" < XXXX@XXXXX.COM >wrote in message
Quote
Borland C++ Builder 6 compiler gives a warning on "suspicious pointer
arithmetic" (W8072) on the following code:

typedef unsigned char ubyte;
typedef __int64 int64;

bool bit_is_set(ubyte bits[], int64 bit)
{
int64 pa = bit / CHAR_BIT;
int64 pb = bit % CHAR_BIT;

if(bits[pa] & (1 << pb)) // compiler warns here
return true;
else
return false;
}

the bits[] is an array of bytes that must serve as a bitmap.

should i ignore the warning or bits[] array indeed cannot be addressed
by int64 value?

Visual C++ 7.0 compiler is silent about this.

Thanks,
Muzaffar
 

Re:suspicious pointer arithmetic

"Andrue Cope [TeamB]" < XXXX@XXXXX.COM >writes:
Quote
Muzaffar Mahkamov wrote:

>Thank you, it's worked fine (no warnings). But afaik, 'unsigned long'
>is also 64-bits wide.

No, it's 32 bits.

'long' means the same as 'int' for BCB6.
Not quite. The sizeof(long) is the same as sizeof(int) for BCB6, but
they are distinctly different types, and are treated differently.
Consider function overloading and templates for a few places where
they are not treated the same.
--
Chris (TeamB);
 

Re:suspicious pointer arithmetic

Chris Uzdavinis (TeamB) wrote:
Quote
Consider function overloading and templates for a few places where
they are not treated the same.
Oh right, thanks.
--
Andrue Cope [TeamB]
[Bicester, Uk]
info.borland.com/newsgroups/guide.html