[build] Fix a clang warning

[build] Fix a clang warning

For cross-compiler-compatibility and standards compliance %p
requires a void*, rather than any pointer type.

BUG=chromium:474921
Committed: https://crrev.com/a23222ed32d8a4829af1dc33f93ca599fef6c50a
Cr-Commit-Position: refs/heads/master@{#36466}

[Michael Achenbach, 2016-05-23 09:30:58]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 09:31:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001073002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001073002/20001

[commit-bot: I haz the power, 2016-05-23 09:34:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 09:35:00]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)

[Michael Achenbach, 2016-05-23 09:52:31]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 09:52:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001073002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001073002/40001

[commit-bot: I haz the power, 2016-05-23 09:56:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 09:56:10]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)

[Michael Achenbach, 2016-05-23 11:50:52]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 11:51:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001073002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001073002/60001

[commit-bot: I haz the power, 2016-05-23 11:55:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 11:55:53]

Dry run: Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/15766)

[Michael Achenbach, 2016-05-23 14:01:45]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 14:01:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001073002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001073002/80001

[commit-bot: I haz the power, 2016-05-23 14:33:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 14:33:32]

Dry run: This issue passed the CQ dry run.

[Michael Achenbach, 2016-05-23 14:54:01]

Description was changed from

==========
[build] Fix a clang warning

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

to

==========
[build] Fix a clang warning

BUG=chromium:474921
==========

[Michael Achenbach, 2016-05-23 14:54:02]

machenbach@chromium.org changed reviewers:
+ jkummerow@chromium.org, vogelheim@chromium.org

[Michael Achenbach, 2016-05-23 14:56:43]

PTAL. Not really sure what I'm doing when I enter c++ land. Is it useful to
clean this up? Is it the right way to do it at all?

https://codereview.chromium.org/2001073002/diff/80001/test/cctest/test-disasm...
File test/cctest/test-disasm-mips64.cc (right):

https://codereview.chromium.org/2001073002/diff/80001/test/cctest/test-disasm...
test/cctest/test-disasm-mips64.cc:139: reinterpret_cast<void
*>(((uint64_t)(progcounter + 1) & ~0xfffffff) |  \
Not sure if that should be rather changed to a static_cast instead?

[vogelheim, 2016-05-23 15:49:52]

On 2016/05/23 14:56:43, Michael Achenbach (slow) wrote:
> PTAL. Not really sure what I'm doing when I enter c++ land. Is it useful to
> clean this up? Is it the right way to do it at all?

Generally speaking yes, since warnings are mostly useful and thus disabling any
warning has the potential to hide some real bugs.

In this case I haven't made up my mind yet: I'm trying to figure out *why* clang
--Wformat-pedantic was warning on these in the first place. If I understand
things right, the use of %p was correct and I don't understand why a cast to
void* would be an improvement here.

[vogelheim, 2016-05-23 16:24:44]

On 2016/05/23 15:49:52, vogelheim wrote:
> On 2016/05/23 14:56:43, Michael Achenbach (slow) wrote:
> > PTAL. Not really sure what I'm doing when I enter c++ land. Is it useful to
> > clean this up? Is it the right way to do it at all?
> 
> Generally speaking yes, since warnings are mostly useful and thus disabling
any
> warning has the potential to hide some real bugs.
> 
> In this case I haven't made up my mind yet: I'm trying to figure out *why*
clang
> --Wformat-pedantic was warning on these in the first place. If I understand
> things right, the use of %p was correct and I don't understand why a cast to
> void* would be an improvement here.

A hint from Manuel (works on LLVM) leads to
llvm/tools/clang/lib/Sema/SemaChecking.cpp:4984 ... I can't really read that
code, but it appears to check for typedef-ed arguments to printf and recommends
to use non-typedef-ed versions instead. If so, I don't really get the point, and
am not sure we should adopt that reasoning.

Of course that assumes I'm reading the LLVM documentation (aka code) correctly,
which is a rather big assumption. Maybe we can briefly discuss?

[vogelheim, 2016-05-23 16:54:59]

From additional discussions w/ Jakob:

Chrome does this, too: https://codereview.chromium.org/1148953002/
Clang introduced this after long debate:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150223/124303.html

[vogelheim, 2016-05-24 06:52:06]

lgtm w/ one nit:

Please change the CL description to explicitly explain what the warning is
about, namely that for cross-compiler-compatibility and standards compliance %p
requires a void*, rather than any pointer type.


My reasons, in case anyone is following this: :)
Following the discussion w/ Jakob, lgtm because
- Chrome does this,
- there was a lengthy discussion among clang-devs, and consensus there was this
was favorable,
- the clang discussion (referenced in one of the links in the CL comments)
explicitly references C + C++ standards which seem to require this behaviour,
too.

[vogelheim, 2016-05-24 06:52:23]

https://codereview.chromium.org/2001073002/diff/80001/test/cctest/test-disasm...
File test/cctest/test-disasm-mips64.cc (right):

https://codereview.chromium.org/2001073002/diff/80001/test/cctest/test-disasm...
test/cctest/test-disasm-mips64.cc:139: reinterpret_cast<void
*>(((uint64_t)(progcounter + 1) & ~0xfffffff) |  \
On 2016/05/23 14:56:43, Michael Achenbach (slow) wrote:
> Not sure if that should be rather changed to a static_cast instead?

Probably not. This converts integers to pointers; I don't think static_cast
would let you do that. One could argue this shouldn't use %p in the first place,
but I'm not sure that's a desirable change.

[Michael Achenbach, 2016-05-24 07:14:57]

Description was changed from

==========
[build] Fix a clang warning

BUG=chromium:474921
==========

to

==========
[build] Fix a clang warning

For cross-compiler-compatibility and standards compliance %p
requires a void*, rather than any pointer type.

BUG=chromium:474921
==========

[Michael Achenbach, 2016-05-24 07:17:40]

On 2016/05/24 06:52:06, vogelheim wrote:
> lgtm w/ one nit:
> 
> Please change the CL description to explicitly explain what the warning is
> about, namely that for cross-compiler-compatibility and standards compliance
%p
> requires a void*, rather than any pointer type.
> 
> 
> My reasons, in case anyone is following this: :)
> Following the discussion w/ Jakob, lgtm because
> - Chrome does this,
> - there was a lengthy discussion among clang-devs, and consensus there was
this
> was favorable,
> - the clang discussion (referenced in one of the links in the CL comments)
> explicitly references C + C++ standards which seem to require this behaviour,
> too.

Done. I'll also commit the gyp change in a separate CL. This CL is too large,
but will stick without deactivating the warning. The latter might cause more
problems in case I forgot a place, and I don't want to revert/reland the whole
thing.

[Michael Achenbach, 2016-05-24 07:18:44]

The CQ bit was checked by machenbach@chromium.org

[Michael Achenbach, 2016-05-24 07:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2001073002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-24 07:18:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001073002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001073002/120001

[commit-bot: I haz the power, 2016-05-24 07:25:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 07:25:48]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/15831)

[Michael Achenbach, 2016-05-24 07:27:22]

Patchset #8 (id:140001) has been deleted

[Michael Achenbach, 2016-05-24 07:29:18]

machenbach@chromium.org changed reviewers:
+ bmeurer@chromium.org, mlippautz@chromium.org

[Michael Achenbach, 2016-05-24 07:29:19]

PTAL owners of:

    src/ast/prettyprinter.cc
    src/heap/mark-compact.cc
    src/heap/spaces-inl.h
    src/heap/spaces.cc
    src/ic/arm/ic-arm.cc
    src/ic/arm64/ic-arm64.cc
    src/ic/ia32/ic-ia32.cc
    src/ic/mips/ic-mips.cc
    src/ic/mips64/ic-mips64.cc
    src/ic/ppc/ic-ppc.cc
    src/ic/s390/ic-s390.cc
    src/ic/x64/ic-x64.cc
    src/ic/x87/ic-x87.cc

[Benedikt Meurer, 2016-05-24 07:30:24]

LGTM.

[Michael Lippautz, 2016-05-24 10:41:22]

lgtm

[Michael Achenbach, 2016-05-24 10:42:33]

The CQ bit was checked by machenbach@chromium.org

[Michael Achenbach, 2016-05-24 10:42:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2001073002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-24 10:42:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001073002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001073002/120001

[commit-bot: I haz the power, 2016-05-24 10:45:28]

Description was changed from

==========
[build] Fix a clang warning

For cross-compiler-compatibility and standards compliance %p
requires a void*, rather than any pointer type.

BUG=chromium:474921
==========

to

==========
[build] Fix a clang warning

For cross-compiler-compatibility and standards compliance %p
requires a void*, rather than any pointer type.

BUG=chromium:474921
==========

[commit-bot: I haz the power, 2016-05-24 10:45:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-24 10:47:30]

Description was changed from

==========
[build] Fix a clang warning

For cross-compiler-compatibility and standards compliance %p
requires a void*, rather than any pointer type.

BUG=chromium:474921
==========

to

==========
[build] Fix a clang warning

For cross-compiler-compatibility and standards compliance %p
requires a void*, rather than any pointer type.

BUG=chromium:474921

Committed: https://crrev.com/a23222ed32d8a4829af1dc33f93ca599fef6c50a
Cr-Commit-Position: refs/heads/master@{#36466}
==========

[commit-bot: I haz the power, 2016-05-24 10:47:34]

Patchset 7 (id:??) landed as
https://crrev.com/a23222ed32d8a4829af1dc33f93ca599fef6c50a
Cr-Commit-Position: refs/heads/master@{#36466}