974272 - gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp:17:40: error: unknown type name 'cpu_set_t'

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jan Beich]

Bug 890240 were dropped on update leading to

gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp:17:40: error:
      unknown type name 'cpu_set_t'
static int nth_set_cpu(unsigned int n, cpu_set_t* cpuSet) {
                                       ^
gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp:35:5: error:
      unknown type name 'cpu_set_t'
    cpu_set_t parentCpuset;
    ^
gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp:36:60: error:
      use of undeclared identifier 'cpu_set_t'
    if (0 != pthread_getaffinity_np(pthread_self(), sizeof(cpu_set_t), &pare...
                                                           ^
gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp:40:5: error:
      unknown type name 'cpu_set_t'
    cpu_set_t cpuset;
    ^
gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp:44:47: error:
      use of undeclared identifier 'cpu_set_t'; did you mean 'cpuset'?
                                       sizeof(cpu_set_t),
                                              ^~~~~~~~~
                                              cpuset
gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp:40:15: note:
      'cpuset' declared here
    cpu_set_t cpuset;
              ^
5 errors generated.
gmake[1]: *** [SkThreadUtils_pthread_linux.o] Error 1

Comment 1

[Jan Beich]

Attachment: re-apply fix

NetBSD now uses SkThreadUtils_pthread_other.cpp. Its cpuset_create() API in attachment 827385 [details] [diff] [review] should probably be in separate file unless someone figures enough macro magic to reduce ugly ifdefs.

Can someone push to Try as well?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8378097 [details] [diff] [review]
re-apply fix

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

::: gfx/skia/moz.build
@@ -706,5 @@
>          'trunk/src/ports/SkTime_Unix.cpp',
>          'trunk/src/ports/SkTLS_pthread.cpp',
>          'trunk/src/utils/android/ashmem.cpp',
>          'trunk/src/utils/SkThreadUtils_pthread.cpp',
> -        'trunk/src/utils/SkThreadUtils_pthread_other.cpp',

moz.build is generated from generate_mozbuild.py now, so you'll need to make these changes there instead, otherwise they will be dropped on the next rebase (which is ideally in 6 weeks or so).

::: gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp
@@ +12,5 @@
>  #include "SkThreadUtils.h"
>  #include "SkThreadUtils_pthread.h"
>  
>  #include <pthread.h>
> +#ifdef __FreeBSD__

Why is there BSD stuff in a linux-specific file? Shouldn't this be in _other?

Comment 3

[Jan Beich]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> ::: gfx/skia/trunk/src/utils/SkThreadUtils_pthread_linux.cpp
> @@ +12,5 @@
> >  #include "SkThreadUtils.h"
> >  #include "SkThreadUtils_pthread.h"
> >  
> >  #include <pthread.h>
> > +#ifdef __FreeBSD__
> 
> Why is there BSD stuff in a linux-specific file? Shouldn't this be in _other?

Because the API is identical except for s/cpu_set_t/cpuset_t/. This makes it easier to stay in sync on updates. And CPU_COUNT may sometimes be undefined on Linux.

_other.cpp is a stub for platforms that don't implement CPU binding.

Comment 4

[George Wright (:gw280) (needinfo me!)]

Upstream didn't like this patch: https://codereview.chromium.org/22259005/

I'm going to look at their suggestion of not building SkThreadUtils and see if that works.

Comment 5

[Jan Beich]

Attachment: re-apply bug890240 version

This time with generate_mozbuild.py.

Comment 6

[Jan Beich]

Attachment: only stub version

Only one version needs r+.

Comment 7

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8378383 [details] [diff] [review]
only stub version

Looks fine to me. Let's run it through try first though to ensure we don't break anything (I don't think it will).

Comment 8

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8378383 [details] [diff] [review]
only stub version

Oops, sorry, misread the patch. You should add SkThreadUtils_pthread_other to the pre-populated separated dictionary in generate_separated_sources, rather than where you currently have it.

Comment 9

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8378382 [details] [diff] [review]
re-apply bug890240 version

As discussed with upstream, we have decided this isn't the right way to go.

Comment 10

[Jan Beich]

Attachment: only stub version

Regen, current moz.build isn't clean output from generate_mozbuild.py.

 diff --git a/gfx/skia/moz.build b/gfx/skia/moz.build
 index 0f4b271..39f9802 100644
 --- a/gfx/skia/moz.build
 +++ b/gfx/skia/moz.build
 @@ -697,6 +697,7 @@
      SOURCES += [
          'trunk/src/images/SkImageRef_ashmem.cpp',
          'trunk/src/ports/SkDebug_android.cpp',
 +        'trunk/src/ports/SkFontHost_android_old.cpp',
          'trunk/src/ports/SkFontHost_cairo.cpp',
          'trunk/src/ports/SkFontHost_FreeType.cpp',
          'trunk/src/ports/SkFontHost_FreeType_common.cpp',
 @@ -707,6 +708,7 @@
          'trunk/src/utils/android/ashmem.cpp',
          'trunk/src/utils/SkThreadUtils_pthread.cpp',
      ]
 +    DEFINES['SK_FONTHOST_CAIRO_STANDALONE'] = 0
  if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
      SOURCES += [
          'trunk/src/ports/SkDebug_stdio.cpp',
 @@ -833,7 +835,7 @@
  if CONFIG['INTEL_ARCHITECTURE'] and CONFIG['HAVE_TOOLCHAIN_SUPPORT_MSSSE3']:
      DEFINES['SK_BUILD_SSSE3'] = 1

 -if (CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android') or    (CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa') or    CONFIG['MOZ_WIDGET_GTK']:
 +if (CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android') or (CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk') or    (CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa') or    CONFIG['MOZ_WIDGET_GTK']:
      DEFINES['SK_FONTHOST_DOES_NOT_USE_FONTMGR'] = 1

  if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows':

Comment 11

[Jan Beich]

(In reply to George Wright (:gw280) from comment #8)
> Comment on attachment 8378383 [details] [diff] [review]
> only stub version
> 
> Oops, sorry, misread the patch. You should add SkThreadUtils_pthread_other
> to the pre-populated separated dictionary in generate_separated_sources,
> rather than where you currently have it.

If I put it under |common| it'd break Windows. Only XP_UNIX platforms define SK_USE_POSIX_THREADS.

How to negate with dictionary?

Comment 12

[George Wright (:gw280) (needinfo me!)]

I think you need to list the same file under every item except common and windows

Comment 13

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8378386 [details] [diff] [review]
only stub version

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

Clearing the r? flag as per previous r- comment

Comment 14

[Jan Beich]

Attachment: drop setProcessorAffinity() version

Why not drop setProcessorAffinity() implementation altogether. It's not used in Mozilla tree while upstream usage is limited to
  trunk/tests/RefCntTest.cpp
  trunk/tests/AtomicTest.cpp

I want to avoid listing stub for every supported XP_UNIX: linux, mac, solaris, hurd, dragonfly, freebsd, gnukfreebsd, netbsd, openbsd and maybe more.

Comment 15

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8378753 [details] [diff] [review]
drop setProcessorAffinity() version

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

Yep, that makes a lot more sense :)

Comment 16

[Jan Beich]

Land only r+ patch, don't obsolete r- one. No Try run done because builds fine on one pthread platform.

Comment 17

[Jan Beich]

Attachment: remove from bug890240 patch from gfx/skia/patches/

Do we need to keep the obsolete patch ?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d30842fedb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d098abf43ad

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2d30842fedb
https://hg.mozilla.org/mozilla-central/rev/6d098abf43ad

Comment 20

[Jan Beich]

Attachment: drop setProcessorAffinity() qt followup

Bug 974335 slipped in SkThreadUtils_pthread_linux.cpp that was supposed to be in blacklist. Regen.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11069517746f

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/11069517746f