656008 - [vmbase] vmpi_thread_t is not universally pointer type; NULL not universal option.

Status: RESOLVED FIXED

(Tamarin Graveyard :: API, defect, P2)

Description

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

We have a couple spots in the code where we assign NULL to a member of type vmpi_thread_t (or compare it against NULL, etc).

This is not generally valid.  On some host environments, vmpi_thread_t is phtread_t, and that in turn is an table-index, not a pointer.

See for example:

http://freebsd.monkey.org/freebsd-threads/200307/msg00063.html

Comment 1

[William Maddox]

I was just about to file a duplicate of this.  It's causing Linux build failures on our new Hudson-based sandbox builder, presumably due to differences in the build entironment: different compiler version (observed), thread library (conjectured), or compiler option settings (observed).

Comment 2

[Tommy Reilly]

We shouldn't be setting a vmpi_thread_t to NULL or 0, we should have VMPI_NULL_THREAD or something.

Comment 3

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

(In reply to comment #2)
> We shouldn't be setting a vmpi_thread_t to NULL or 0, we should have
> VMPI_NULL_THREAD or something.

I'd be fine with that but of course the problem is determining what an appropriate expression is to assign to VMPI_NULL_THREAD.

One nasty hack I've been considering changing VMPI_threadCreate so that it checks if the result vmpi_thread_t is == 0, and if so, it allocates yet another thread (which should be guaranteed to get a nonzero vmpi_thread_t), then releasing the one that got the 0 value.  Its sort of an embarrassing hack, but it should be portable.  Thoughts?

Comment 4

[Dan Smith]

Comment from Tommy on bug related to this issue:


Hi,

I'm looking at a compile error on GCC4.5. It complains about setting NULL to a non pointer type.

On linux vmpi_thread_t is defined as pthread_t. That's a long int, not a pointer, so the error.

/home/ambry/flash/split/third_party/avmplus/MMgc/GC.cpp: In member function ‘void MMgc::GC::ThreadLeave(bool, MMgc::GC*)’:
/home/ambry/flash/split/third_party/avmplus/MMgc/GC.cpp:3495:26: error: converting to non-pointer type ‘vmpi_thread_t’ from NULL
make[2]: *** [CMakeFiles/mmgc.dir/home/ambry/flash/split/third_party/avmplus/MMgc/GC.cpp.o] Error 1
make[1]: *** [CMakeFiles/mmgc.dir/all] Error 2

Comment 5

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

(In reply to Felix S Klock II from comment #3)
> One nasty hack I've been considering changing VMPI_threadCreate so that it
> checks if the result vmpi_thread_t is == 0, and if so, it allocates yet
> another thread (which should be guaranteed to get a nonzero vmpi_thread_t),
> then releasing the one that got the 0 value.  Its sort of an embarrassing
> hack, but it should be portable.  Thoughts?

Maybe I misremember the posix thread API, but I'm now thinking that the above hack won't work in cases where the prechosen value (i.e. 0 above) happens to match the thread id assigned to the main thread.  (I'm just noting this as I pick up this bug as an small weekend task.)

Comment 6

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

Attachment: patch A v0: initial draft. not correct (see comments).

Comment 7

[Trevor Baker]

Glad to see some work happening on this.  Retargeting to Brannan as P2.

Comment 8

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

Attachment: patch A v1: use -1 on linux and NULL elsewhere.

I'm not thrilled with this hack, but I also don't have any immediate alternative solutions.

I am doing the access to the null value via a method so that I can include assertions to try to ensure that we find out about cases where the assumption about -1 working ASAP.

(I considered more elaborate schemes, such as creating a thread just so that we have a unique identifier that we know no other thread identifier will match.  Keeping this in a method will let us introduce such schemes in the future if necessary, but I hope it won't ever become necessary.)

Comment 9

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

Comment on attachment 594976 [details] [diff] [review]
patch A v1: use -1 on linux and NULL elsewhere.

Ed: feel free to refile review request as you like, if you think someone else will care more about the state of Linux and/or vmpi_thread_t.  (Krzysztof is one obvious candidate, if he has time.)  I do not plan to attempt to land this until it gets R+'ed.

Brent: I do hope to put this through paces on the buildbot; if there are other linux hosts that we might care about, such as cloud-hosted virtual machines or what not, I would appreciate you checking out how well things work there.  (Do note that the very-newest GCC versions may give you trouble; see Bug 559696, comment 10.  I had to explicitly downgrade to 4.5 in my Ubunutu VMware image.)

Comment 10

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

(In reply to Felix S Klock II from comment #8)
> I am doing the access to the null value via a method so that I can include
> assertions to try to ensure that we find out about cases where the
> assumption about -1 working ASAP.

The above should have ended with "... find out about cases where the assumption (about -1 working) breaks ASAP."

Comment 11

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

Comment on attachment 594976 [details] [diff] [review]
patch A v1: use -1 on linux and NULL elsewhere.

(retracting review/feedback requests; the patch was only tested on linux and did not build properly on mac; don't know yet about windows.  Will wait until I've revised patch and checked all three before reposting requests.)

Comment 12

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

Attachment: patch A v2: use -1 on linux and NULL elsewhere

(fix for mac was simple, just needed to move the new function definition down the header.  the windows patch didn't suffer the same mistake, so i might get lucky and not need to revise it further.  but am sticking to my assertion that i will build first and request reviews later.)

Comment 13

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

Comment on attachment 594998 [details] [diff] [review]
patch A v2: use -1 on linux and NULL elsewhere

same notes as in comment 9 apply: Redirect r?/f? request as you like, and I do not attempt to land this until it gets R+'ed by someone.

At this point I have confirmed it builds on Mac, Unix, and Windows, and also put it through a buildbot run (which passed!).

Comment 14

[Krzysztof Palacz]

Comment on attachment 594998 [details] [diff] [review]
patch A v2: use -1 on linux and NULL elsewhere

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

Looks good to me, spotted some nits.

::: VMPI/ThreadsPosix-inlines.h
@@ +91,5 @@
>  
>  REALLY_INLINE vmpi_thread_t VMPI_currentThread()
>  {
> +    vmpi_thread_t rtn = pthread_self();
> +    assert(rtn != VMPI_nullThread());

see the comment in ThreadsPosix.cpp

::: VMPI/ThreadsPosix.cpp
@@ +65,5 @@
>  
>  bool VMPI_threadCreate(vmpi_thread_t* thread, vmpi_thread_attr_t* attr, vmpi_thread_start_t start_fn, vmpi_thread_arg_t arg)
>  {
> +    bool rtn = pthread_create(thread, attr, start_fn, arg) == 0;
> +    assert(*thread != VMPI_nullThread());

This assert could be puzzling without a comment, because it tests the validity of the VMPI_nullThread() implementation, not the validity of the implementation of the current function.

::: vmbase/VMThread.cpp
@@ -241,4 +241,4 @@
> >      public:
> >          static void sleep(int32_t timeout)
> >          {
> > -            VMPI_callWithRegistersSaved(sleepInSafepointGate, (void*)timeout);
> > +            VMPI_callWithRegistersSaved(sleepInSafepointGate, (void*)(intptr_t)timeout);

Is this change related to the topic of the patch?

Comment 15

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

(In reply to Krzysztof Palacz from comment #14)
> >  bool VMPI_threadCreate(vmpi_thread_t* thread, vmpi_thread_attr_t* attr, vmpi_thread_start_t start_fn, vmpi_thread_arg_t arg)
> >  {
> > +    bool rtn = pthread_create(thread, attr, start_fn, arg) == 0;
> > +    assert(*thread != VMPI_nullThread());
> 
> This assert could be puzzling without a comment, because it tests the
> validity of the VMPI_nullThread() implementation, not the validity of the
> implementation of the current function. 

Good point.  I'll add a note to the assertions that will explain their purpose and also point the reader to this ticket.

> ::: vmbase/VMThread.cpp
> @@ -241,4 +241,4 @@
> > >      public:
> > >          static void sleep(int32_t timeout)
> > >          {
> > > -            VMPI_callWithRegistersSaved(sleepInSafepointGate, (void*)timeout);
> > > +            VMPI_callWithRegistersSaved(sleepInSafepointGate, (void*)(intptr_t)timeout);
> 
> Is this change related to the topic of the patch?

No, I think that is left-over from an aborted attempt i made to get things building with GCC 4.6.

Comment 16

[Tamarin Bot]

changeset: 7245:e82a27378c06
user:      Felix Klock II <[e-mail]>
summary:   Bug 656008: VMPI_nullThread, using -1 on linux and NULL elsewhere (r=kpalacz).

http://hg.mozilla.org/tamarin-redux/rev/e82a27378c06