Closed Bug 1103827 Opened 5 years ago Closed 5 years ago

[gonk-l] NUWA get link error

Categories

(Firefox OS Graveyard :: GonkIntegration, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: seinlin, Assigned: seinlin)

References

Details

Attachments

(1 file, 3 obsolete files)

__real___pthread_cond_timedwait and __pthread_cond_timedwait are not found.
Blocks: gonk-L
Attached patch bug-1103827.patch (obsolete) — Splinter Review
Cervantes, Could you have a look to this patch? I am not sure if it is reasonable to disable them for api level 21 (gonk-l).
Attachment #8528150 - Flags: feedback?(cyu)
Comment on attachment 8528150 [details] [diff] [review]
bug-1103827.patch

I am not a fan of ANDROID_VERSION checks in the code base. I searched usage of __pthread_cond_timedwait() and no one is (and shouldn't be) using this nonstandard function. I think we should just remove __wrap_pthread_cond_timedwait() and declarations of __pthread_cond_timedwait() and __real___pthread_cond_timedwait().
Attachment #8528150 - Flags: feedback?(cyu) → feedback-
Cyu,

I verified on try server, remove __real___pthread_cond_timedwait will cause compile error.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9eebfc65c9f2

So, I think we should only remove __wrap_pthread_cond_timedwait() and declarations of __pthread_cond_timedwait(). 

How do you think?
Flags: needinfo?(cyu)
Remove __wrap_pthread_cond_timedwait() and declarations of __pthread_cond_timedwait() will also breaks the build.

https://tbpl.mozilla.org/?tree=Try&rev=bd712aeceae6
(In reply to Kai-Zhen Li [:seinlin] from comment #3)
> Cyu,
> 
> I verified on try server, remove __real___pthread_cond_timedwait will cause
> compile error.
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9eebfc65c9f2
> 
> So, I think we should only remove __wrap_pthread_cond_timedwait() and
> declarations of __pthread_cond_timedwait(). 
> 
> How do you think?

Yeah, please go ahead with removal of __pthread_cond_timed_wait() and its wrapper. It's not supposed to be used by any means. Also please remove the LD flag --wrap=__pthread_cond_timedwait in configure.in.
Flags: needinfo?(cyu)
Is __pthread_cond_timedwait different from pthread_cond_timedwait?  Are you sure it's not used?  I think we have uses of it in the code base.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> Is __pthread_cond_timedwait different from pthread_cond_timedwait?  Are you
__pthread_cond_timedwait() is not part of the pthreads standard. glibc has it as a strong alias:
strong_alias (__pthread_cond_timedwait, pthread_cond_timedwait)
so it's functionally identical.

In bionic libc before L, __pthread_cond_timedwait() has an extra parameter for specify the clock to use. So even the 2 libraries has __pthread_cond_timedwait() the signatures are different. After L bionic just declares it file static. I think it's accceptable.

Given that I don't think we should ever call __pthread_cond_timedwait().

> sure it's not used?  I think we have uses of it in the code base.
No at least from DXR.
Attached patch bug-1103827.patch (obsolete) — Splinter Review
Cyu, Could you review this patch? I verified on try server and it will not break any build.

Try result: https://tbpl.mozilla.org/?tree=Try&rev=9e7e1fe9b5ec
Attachment #8528150 - Attachment is obsolete: true
Attachment #8531868 - Flags: review?(cyu)
Comment on attachment 8531868 [details] [diff] [review]
bug-1103827.patch

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

r- because you also removed __wrap_pthread_cond_timedwait() that we need. Please only remove __real___pthread_cond_timedwait() and __wrap___pthread_cond_timed_wait() (notice the number of underscores).

::: configure.in
@@ +7288,1 @@
>      fi

You need to keep --wrap=pthread_cond_timedwait

::: mozglue/build/Nuwa.cpp
@@ -53,5 @@
>                        int timeout);
>  int __real_pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mtx);
> -int __real_pthread_cond_timedwait(pthread_cond_t *cond,
> -                                  pthread_mutex_t *mtx,
> -                                  const struct timespec *abstime);

You need to keep this declaration.

@@ -1121,5 @@
> -  THREAD_FREEZE_POINT2_VIP();
> -
> -  return rv;
> -}
> -

You need to keep this wrapper.
Attachment #8531868 - Flags: review?(cyu) → review-
Attached patch bug-1103827.patch (obsolete) — Splinter Review
Cervantes, I updated a new patch, could you have a look?
Attachment #8531868 - Attachment is obsolete: true
Attachment #8535531 - Flags: review?(cyu)
Comment on attachment 8535531 [details] [diff] [review]
bug-1103827.patch

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

Please also clean up --wrap=__pthread_cond_timedwait in configure.in.
Attachment #8535531 - Flags: review?(cyu) → feedback+
Cervantes, --wrap=__pthread_cond_timedwait is removed. Could you review this patch? Thanks!

Try result: https://tbpl.mozilla.org/?tree=Try&rev=07be5b6c19eb
Attachment #8535531 - Attachment is obsolete: true
Attachment #8536366 - Flags: review?(cyu)
Attachment #8536366 - Flags: review?(cyu) → review+
https://hg.mozilla.org/mozilla-central/rev/7453c9e6a353
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.