Fix truncation warning in PEImage on VS2015

Fix truncation warning in PEImage on VS2015

d:\src\cr3\src\base\win\pe_image.h(250): error C2220: warning treated as error - no 'object' file generated
d:\src\cr3\src\base\win\pe_image.h(250): warning C4302: 'reinterpret_cast': truncation from 'LPCSTR' to 'WORD'

Separate ptr->int from truncation to WORD.

Also, while we're here, fix a cast in IsOrdinal.

R=cpu@chromium.org
BUG=440500
Committed: https://crrev.com/85fd8b5480d2eb4a2f85f8bc6365969a6de259c3
Cr-Commit-Position: refs/heads/master@{#313543}

[grt (UTC plus 2), 2015-01-27 20:55:56]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2015-01-27 20:55:56]

https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
File base/win/pe_image.h (right):

https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
should this be
   return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
? note the 'u' so 0x80000123 isn't considered an ordinal.

[Will Harris, 2015-01-27 21:02:00]

On 2015/01/27 20:55:56, grt wrote:
> https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
> File base/win/pe_image.h (right):
> 
>
https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
> base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
> should this be
>    return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
> ? note the 'u' so 0x80000123 isn't considered an ordinal.

Pointer to an unsigned int is the same as pointer to a signed int since pointers
are always unsigned anyway.  Moreover, the C standard says that it is not
guaranteed that uintptr_t can hold a function pointer.  I would use intptr_t
here personally.

[scottmg, 2015-01-27 21:12:09]

intptr_t == int on x86, right? So wouldn't 0x80000123 turn into a negative
number a fail that comparison?

(Do you want me to change that in this CL or a follow up?)

On Tue, Jan 27, 2015 at 1:01 PM, <wfh@chromium.org> wrote:

> On 2015/01/27 20:55:56, grt wrote:
>
>> https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
>> File base/win/pe_image.h (right):
>>
>
>
> https://codereview.chromium.org/883873002/diff/1/base/win/
> pe_image.h#newcode245
>
>> base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
>> should this be
>>     return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
>> ? note the 'u' so 0x80000123 isn't considered an ordinal.
>>
>
> Pointer to an unsigned int is the same as pointer to a signed int since
> pointers
> are always unsigned anyway.  Moreover, the C standard says that it is not
> guaranteed that uintptr_t can hold a function pointer.  I would use
> intptr_t
> here personally.
>
>
> https://codereview.chromium.org/883873002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[grt (UTC plus 2), 2015-01-27 21:16:58]

On 2015/01/27 21:02:00, Will Harris wrote:
> On 2015/01/27 20:55:56, grt wrote:
> > https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
> > File base/win/pe_image.h (right):
> > 
> >
>
https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
> > base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
> > should this be
> >    return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
> > ? note the 'u' so 0x80000123 isn't considered an ordinal.
> 
> Pointer to an unsigned int is the same as pointer to a signed int since
pointers
> are always unsigned anyway.  Moreover, the C standard says that it is not
> guaranteed that uintptr_t can hold a function pointer.  I would use intptr_t
> here personally.

I haven't read the legalese, but if the spec is saying that you can round-trip
from pointer to intptr_t then back to pointer safely, that isn't the same as
saying that intptr_t is unsigned. If intptr_t is a signed integer, then "<=
0xFFFF" can be true for intptr_t-from-pointer at the top half of the address
space.

[Will Harris, 2015-01-27 21:19:00]

On 2015/01/27 21:12:09, scottmg wrote:
> intptr_t == int on x86, right? So wouldn't 0x80000123 turn into a negative
> number a fail that comparison?

...but the static cast will just cast it back to unsigned so it doesn't matter. 
The generated code should be identical whether you use uintptr_t or intptr_t so
it's just whether you believe function pointers should be stored in uintptr_t (C
standard says "no").

[grt (UTC plus 2), 2015-01-27 21:20:25]

On 2015/01/27 21:19:00, Will Harris wrote:
> On 2015/01/27 21:12:09, scottmg wrote:
> > intptr_t == int on x86, right? So wouldn't 0x80000123 turn into a negative
> > number a fail that comparison?
> 
> ...but the static cast will just cast it back to unsigned so it doesn't
matter. 
> The generated code should be identical whether you use uintptr_t or intptr_t
so
> it's just whether you believe function pointers should be stored in uintptr_t
(C
> standard says "no").

Please refer to line 245, which compares the integer representation of a pointer
with a small positive integer. This is where I think the uintptr_t matters.

[Will Harris, 2015-01-27 21:22:03]

oh sorry as usual I was looking at the wrong code (the code below with the
static cast)

[Will Harris, 2015-01-27 21:36:03]

wfh@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2015-01-27 21:36:04]

https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
File base/win/pe_image.h (right):

https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
On 2015/01/27 20:55:56, grt wrote:
> should this be
>    return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
> ? note the 'u' so 0x80000123 isn't considered an ordinal.

my understanding was that 0x80000001 was an ordinal, but the ordinal was 1.

so a check for IsOrdinal should be something like checking the high bit e.g. &
0x80000000 on 32-bit and 0x8000000000000000 on 64-bit.

see http://www.openwatcom.org/ftp/devel/docs/pecoff.pdf section 6.4.2

[scottmg, 2015-01-27 21:41:57]

On 2015/01/27 21:36:04, Will Harris wrote:
> https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
> File base/win/pe_image.h (right):
> 
>
https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
> base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
> On 2015/01/27 20:55:56, grt wrote:
> > should this be
> >    return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
> > ? note the 'u' so 0x80000123 isn't considered an ordinal.
> 
> my understanding was that 0x80000001 was an ordinal, but the ordinal was 1.
> 
> so a check for IsOrdinal should be something like checking the high bit e.g. &
> 0x80000000 on 32-bit and 0x8000000000000000 on 64-bit.
> 
> see http://www.openwatcom.org/ftp/devel/docs/pecoff.pdf section 6.4.2

I think that's only for the import table. IsOrdinal is only used by
GetProcOrdinal, which is only used by GetExportEntry. And, per your link those
appear to be a table of 16 bit values.

[scottmg, 2015-01-27 21:42:04]

https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
File base/win/pe_image.h (right):

https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
On 2015/01/27 20:55:56, grt wrote:
> should this be
>    return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
> ? note the 'u' so 0x80000123 isn't considered an ordinal.

Done.

[scottmg, 2015-01-27 21:45:32]

On 2015/01/27 21:41:57, scottmg wrote:
> On 2015/01/27 21:36:04, Will Harris wrote:
> > https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
> > File base/win/pe_image.h (right):
> > 
> >
>
https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
> > base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
> > On 2015/01/27 20:55:56, grt wrote:
> > > should this be
> > >    return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
> > > ? note the 'u' so 0x80000123 isn't considered an ordinal.
> > 
> > my understanding was that 0x80000001 was an ordinal, but the ordinal was 1.
> > 
> > so a check for IsOrdinal should be something like checking the high bit e.g.
&
> > 0x80000000 on 32-bit and 0x8000000000000000 on 64-bit.
> > 
> > see http://www.openwatcom.org/ftp/devel/docs/pecoff.pdf section 6.4.2
> 
> I think that's only for the import table. IsOrdinal is only used by
> GetProcOrdinal, which is only used by GetExportEntry. And, per your link those
> appear to be a table of 16 bit values.

Er, wait. Maybe not. Now I'm not sure. whargarbl.

[scottmg, 2015-01-27 22:08:09]

On 2015/01/27 21:45:32, scottmg wrote:
> On 2015/01/27 21:41:57, scottmg wrote:
> > On 2015/01/27 21:36:04, Will Harris wrote:
> > > https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
> > > File base/win/pe_image.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
> > > base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
> > > On 2015/01/27 20:55:56, grt wrote:
> > > > should this be
> > > >    return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
> > > > ? note the 'u' so 0x80000123 isn't considered an ordinal.
> > > 
> > > my understanding was that 0x80000001 was an ordinal, but the ordinal was
1.
> > > 
> > > so a check for IsOrdinal should be something like checking the high bit
e.g.
> &
> > > 0x80000000 on 32-bit and 0x8000000000000000 on 64-bit.
> > > 
> > > see http://www.openwatcom.org/ftp/devel/docs/pecoff.pdf section 6.4.2
> > 
> > I think that's only for the import table. IsOrdinal is only used by
> > GetProcOrdinal, which is only used by GetExportEntry. And, per your link
those
> > appear to be a table of 16 bit values.
> 
> Er, wait. Maybe not. Now I'm not sure. whargarbl.

I see the high-bit-set stuff you mentioned but looking at, e.g. the test code

https://code.google.com/p/chromium/codesearch#chromium/src/base/win/pe_image_...

it seems like values <= 0xffff are intended to be an index and "the rest" are
treated as pointers. So I think it's OK as-is.

[cpu_(ooo_6.6-7.5), 2015-01-28 02:52:01]

cpu@chromium.org changed reviewers:
- cpu@chromium.org

[grt (UTC plus 2), 2015-01-28 14:09:51]

On 2015/01/27 22:08:09, scottmg wrote:
> On 2015/01/27 21:45:32, scottmg wrote:
> > On 2015/01/27 21:41:57, scottmg wrote:
> > > On 2015/01/27 21:36:04, Will Harris wrote:
> > > > https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h
> > > > File base/win/pe_image.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/883873002/diff/1/base/win/pe_image.h#newcode245
> > > > base/win/pe_image.h:245: return reinterpret_cast<DWORD>(name) <= 0xFFFF;
> > > > On 2015/01/27 20:55:56, grt wrote:
> > > > > should this be
> > > > >    return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
> > > > > ? note the 'u' so 0x80000123 isn't considered an ordinal.
> > > > 
> > > > my understanding was that 0x80000001 was an ordinal, but the ordinal was
> 1.
> > > > 
> > > > so a check for IsOrdinal should be something like checking the high bit
> e.g.
> > &
> > > > 0x80000000 on 32-bit and 0x8000000000000000 on 64-bit.
> > > > 
> > > > see http://www.openwatcom.org/ftp/devel/docs/pecoff.pdf section 6.4.2
> > > 
> > > I think that's only for the import table. IsOrdinal is only used by
> > > GetProcOrdinal, which is only used by GetExportEntry. And, per your link
> those
> > > appear to be a table of 16 bit values.
> > 
> > Er, wait. Maybe not. Now I'm not sure. whargarbl.
> 
> I see the high-bit-set stuff you mentioned but looking at, e.g. the test code
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/pe_image_...
> 
> it seems like values <= 0xffff are intended to be an index and "the rest" are
> treated as pointers. So I think it's OK as-is.

In the PEImage API, scanning an image's import table (via EnumAllImports)
provides the ordinals as 16-bit values (without the high bit set). The API
expects that looking up an export by ordinal takes just the 16-bit value as
input. So in this case, ignore the high bit stuff.

[grt (UTC plus 2), 2015-01-28 14:12:12]

lgtm

https://codereview.chromium.org/883873002/diff/20001/base/win/pe_image.h
File base/win/pe_image.h (right):

https://codereview.chromium.org/883873002/diff/20001/base/win/pe_image.h#newc...
base/win/pe_image.h:242: return reinterpret_cast<uintptr_t>(name) <= 0xFFFF;
if you wanted intptr_t everywhere for the sake of consistency, this could
become:
  intptr_t value = reinterpret_cast<intptr_t>(name);
  return value >= 0 && value <= 0xFFFF;

[scottmg, 2015-01-28 17:37:17]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-01-28 17:38:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883873002/20001

[commit-bot: I haz the power, 2015-01-28 18:26:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-28 18:26:53]

Patchset 2 (id:??) landed as
https://crrev.com/85fd8b5480d2eb4a2f85f8bc6365969a6de259c3
Cr-Commit-Position: refs/heads/master@{#313543}