663734 - Fix compiler errors on GCC 4.6

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Build Config, defect)

Description

[Jasper St. Pierre]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

gcc (4.5? 4.6?) disallows casting a non-native-sized integer to a (void*) pointer when 'int-to-pointer-cast' is enabled (-Wall triggers it too).

This means that on 64-bit systems, any time int32_t is casted to (void*), it fails to compile.

A patch attached replaces int32_t with intptr_t for all the (void*) errors I saw. The patch also fixes other build errors. It replaces "NULL" for "0" when a pthread handle is used, as NULL is a pointer type, and pthread_t is not, and removes some set-but-unused errors.

Reproducible: Always

Comment 1

[Jasper St. Pierre]

Attachment: build fixes

Comment 2

[Jasper St. Pierre]

Oh, and with these fixes, it builds, but unfortunately doesn't run:

 $ avmshell foo
 ReferenceError: Error #1065: Variable NativeBase is not defined.

Comment 3

[Steven Johnson]

Comment on attachment 538792 [details] [diff] [review]
build fixes

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

Comment 4

[Steven Johnson]

Comment on attachment 538792 [details] [diff] [review]
build fixes

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

R- because of __attribute__((unused)) usage, which will break non-gcc compilers. Otherwise looks good.

::: MMgc/GC.cpp
@@ +3726,4 @@
>              stackEnter = NULL;
>              // cleared so we remain thread ambivalent
>              rememberedStackTop = NULL;
> +            m_gcThread = 0;

IMHO would be nice to define a VMPI_THREAD_NULL type for this, but that's arguably scope creep...

::: extensions/ST_mmgc_basics.st
@@ +307,4 @@
>      GCConfig config;
>      GC *gcb = new GC(GCHeap::GetGCHeap(), config);
>      MMGC_GCENTER(gc);
> +    void *a __attribute__((unused)) = gc->Alloc(8);

Can't use __attribute__((unused)) without an #ifdef or such, it's gcc-specific

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #0)
> It replaces "NULL" for "0" when a pthread handle is used, as NULL is a
> pointer type, and pthread_t is not, and removes some set-but-unused errors.

I'd be wary about this replacement; see Bug 656008 and http://freebsd.monkey.org/freebsd-threads/200307/msg00063.html

Comment 6

[Jasper St. Pierre]

OK, I must have just cloned the repo at the wrong time... applied my patches on 'tip', it works perfectly.

(In reply to comment #4)
> Comment on attachment 538792 [details] [diff] [review] [review]
> build fixes
> 
> Review of attachment 538792 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> R- because of __attribute__((unused)) usage, which will break non-gcc
> compilers. Otherwise looks good.

Our best bet is to modify configure.py to add on 4.5 and 4.6 "-Wno-unused-value", then.

(In reply to comment #5)
> I'd be wary about this replacement; see Bug 656008 and
> http://freebsd.monkey.org/freebsd-threads/200307/msg00063.html

Any other suggestions?

Comment 7

[Dan Smith]

Care to post an updated patch and request a new review?

Comment 8

[Edwin Smith]

Attachment: Remove dead local variable in ArraySort

With this patch, plus -Wno-conversion-null and -Wno-cast-align, gcc-4.6 seems happy.  I haven't done extensive testing yet but linux-x86 acceptance tests seem fine.

Comment 9

[Edwin Smith]

Comment on attachment 569449 [details] [diff] [review]
Remove dead local variable in ArraySort

Correction.  there are other build errors with gcc-4.6, but this at least fixes one of them, no intrinsic reason not to land it.

Comment 10

[Tamarin Bot]

changeset: 6679:1c0daf912860
user:      Edwin Smith <[e-mail]>
summary:   Bug 663734 - Fix compiler errors on GCC 4.6 (r=fklockii+)

http://hg.mozilla.org/tamarin-redux/rev/1c0daf912860

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 538792 [details] [diff] [review]
build fixes

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

::: extensions/ST_mmgc_basics.st
@@ +307,4 @@
>      GCConfig config;
>      GC *gcb = new GC(GCHeap::GetGCHeap(), config);
>      MMGC_GCENTER(gc);
> +    void *a __attribute__((unused)) = gc->Alloc(8);

This objection from Steven is as true now as it was last year.

(We should not be reposting review requests for this patch.  We should instead be revising this patch to address Steven's feedback, perhaps via the means that Jasper outlined in comment 6.)

Comment 12

[smaudet]

Hmm.

I was able to get the latest build of tamarin-redux to build on my machine, however I had to edit configure.py to include '-fno-strict-aliasing -fpermissive', and to remove -Werror

Find the patch attached if you think this is a valid solution, the alternative is a bunch of pointer re-writing - I'm getting quite a few errors without these flags.

Comment 13

[smaudet]

Attachment: Some build fixes

With regards to my last comment on this bug.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.