781552 - Turn on -Werror=int-to-pointer-cast globally

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This is a really good warning. It catches code such as

  int i;
  foo((void*)i);

on platforms where sizeof(int) != sizeof(void*). There is no valid use case for wanting to do that, but sometimes we have to because of bad APIs that want void* arguments; in which case we can still get it to compile by doing

  int i;
  foo((void*)(intptr_t)i);

So, I don't see any downside to turning this on globally (as opposed to just in WARNINGS_AS_ERRORS=1 directories).

See bug 781545 for the original motivation/example.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I do: we don't have tinderbox builds that support that warning yet.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Yeah, that's a blocker.  It seems like it's first in gcc 4.6.

Comment 3

[Mike Hommey [:glandium]]

It's not in clang?

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I've looked for a list of warnings supported by Clang, but annoyingly, I didn't find any.  "man gcc" has a list for gcc (although I don't strictly know if it's complete, and it doesn't give first introduced version).  If you can find a list somewhere, please share!

Comment 5

[Gregory Szorc [:gps]]

(In reply to :Aryeh Gregor from comment #4)
> I've looked for a list of warnings supported by Clang, but annoyingly, I
> didn't find any.  "man gcc" has a list for gcc (although I don't strictly
> know if it's complete, and it doesn't give first introduced version).  If
> you can find a list somewhere, please share!

$ diagtool list-warnings
$ diagtool tree

Comment 6

[sylvestre]

Attachment: fix-bug-781552.diff

The attached patch should fix the bug.

Clang supports this option at from version 3.2 (the 3.4 rc1 is just out).

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8339321 [details] [diff] [review]
fix-bug-781552.diff

Thanks for the patch! This looks totally sensible, but it could stand a run by the try server. Also, we probably want to put this same thing in js/src/configure.in.

Do you have access to push patches to Try?

Comment 8

[sylvestre]

Attachment: fix-bug-781552.diff

Sure, here it is.

but I don't have any permissions :)

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8339338 [details] [diff] [review]
fix-bug-781552.diff

Review of attachment 8339338 [details] [diff] [review]:
-----------------------------------------------------------------

r=ted

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/?tree=Try&rev=00ba77907538

Comment 11

[sylvestre]

Thanks for launching the build.

Some debug builds are broken with this patch:

../../../tools/trace-malloc/tmreader.c: In function 'tmreader_eventloop':
../../../tools/trace-malloc/tmreader.c:527:19: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
../../../tools/trace-malloc/tmreader.c:548:19: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]

I reported the bug 944892

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbebc7b81f0a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbebc7b81f0a