libdisasm: Don't depend on sizeof(void)

libdisasm: Don't depend on sizeof(void)

Due to operator precedence, the address was first cast to void*
and then incremented, which resulted in an error on windows, as
sizeof(void) is undefined and MSVC takes this seriously. Changing
the precedence to perform the addition first.

R=mark@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/104784d8e42a22acadbfa69a55a7f3e0510f8a61

[labath, 2016-01-08 16:16:05]

labath@google.com changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2016-01-08 17:50:55]

Description was changed from

==========
libdisasm: Don't depend on sizeof(void)

Due to operator precedence, the address was first cast to void*
and then incremented, which resulted in an error on windows, as
sizeof(void) is undefined and MSVC takes this seriously. Changing
the precedence to perform the addition first.

BUG=
==========

to

==========
libdisasm: Don't depend on sizeof(void)

Due to operator precedence, the address was first cast to void*
and then incremented, which resulted in an error on windows, as
sizeof(void) is undefined and MSVC takes this seriously. Changing
the precedence to perform the addition first.
==========

[Mark Mentovai, 2016-01-08 17:51:30]

LGTM

[Mark Mentovai, 2016-01-08 17:52:07]

Description was changed from

==========
libdisasm: Don't depend on sizeof(void)

Due to operator precedence, the address was first cast to void*
and then incremented, which resulted in an error on windows, as
sizeof(void) is undefined and MSVC takes this seriously. Changing
the precedence to perform the addition first.
==========

to

==========
libdisasm: Don't depend on sizeof(void)

Due to operator precedence, the address was first cast to void*
and then incremented, which resulted in an error on windows, as
sizeof(void) is undefined and MSVC takes this seriously. Changing
the precedence to perform the addition first.

R=mark@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/104784d8e42a22acadbfa69...
==========

[Mark Mentovai, 2016-01-08 17:52:08]

Committed patchset #1 (id:1) manually as
104784d8e42a22acadbfa69a55a7f3e0510f8a61 (presubmit successful).

[brucedawson, 2016-01-08 21:19:11]

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

[brucedawson, 2016-01-08 21:19:11]

https://codereview.chromium.org/1570843002/diff/1/src/third_party/libdisasm/x...
File src/third_party/libdisasm/x86_disasm.c (right):

https://codereview.chromium.org/1570843002/diff/1/src/third_party/libdisasm/x...
src/third_party/libdisasm/x86_disasm.c:38:
x86_report_error(report_disasm_bounds, (void*)(long)(buf_rva+offset));
Too late for this change, but why are we casting to 'long' here? My guess is
that it is an attempt to convert from uint32_t to pointer-sized integer, but
'long' is not a reliable way to do this since 'long' is always 32-bit on VC++.
The cast to long should be removed or replaced with a cast to intptr_t or,
probably best of all, uintptr_t.

[labath, 2016-01-11 13:23:02]

I agree that the casts look strange, and the long part can probably be removed.
I didn't want to do that as I am new to this code, but I can create a separate
cl if you think it's needed.

[brucedawson, 2016-01-11 18:10:53]

On 2016/01/11 13:23:02, labath wrote:
> I agree that the casts look strange, and the long part can probably be
removed.
> I didn't want to do that as I am new to this code, but I can create a separate
> cl if you think it's needed.

The code is confusing, but not currently harmful. Since both buf_rva and offset
are 32-bit values there is no harm in casting them to long, it's just... odd. It
also might trigger warnings when compiling in 64-bit mode, especially with VC++
2015. But, I don't see any compelling need for refactoring so "leave well enough
alone" may apply.