[Bug other/58996] [4.9 regression] build failure in libcilkrts

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.9.0

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |build
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-11-09
     Ever confirmed|0                           |1

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[bviyer at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

Balaji V. Iyer <bviyer at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bviyer at gmail dot com

--- Comment #2 from Balaji V. Iyer <bviyer at gmail dot com> ---
Hi Paul,
    We do not have a Red Hat 8 machine readily available, and are in the
process of getting one. In the mean time, can you please let me know if the
following fix works for you?

Thanks,

Balaji V. Iyer.

diff --git a/libcilkrts/runtime/os-unix.c b/libcilkrts/runtime/os-unix.c
index dbca21f..eabb17b 100644
--- a/libcilkrts/runtime/os-unix.c
+++ b/libcilkrts/runtime/os-unix.c
@@ -311,6 +311,15 @@ static pid_t linux_gettid(void)
  */
 static int linux_get_affinity_count (int tid)
 {
+#if ! defined __cpu_set_t_defined
+      // Returning 0 will cause __cilkrts_hardware_cpu_count to return the
+      // value from sysconf(_SC_NPROCESSORS_ONLN).
+      //
+      // This should work around the problem with cpu_set_t, CPU_SETSIZE and
+      // CPU_ISSET not being defined on Red Hat 8.
+      return 0;
+#else
+
     cpu_set_t process_mask;

     // Extract the thread affinity mask
@@ -337,6 +346,7 @@ static int linux_get_affinity_count (int tid)
     }

     return available_procs;
+#endif // ! defined __cpu_set_t_defined
 }
 #endif

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
__cpu_set_t_defined is a glibc internal macro, so I don't think it is a good
idea to use it.

I'd suggest you look at libgomp's affinity related configury and ifdefs.
HAVE_PTHREAD_AFFINITY_NP test in libgomp/configure* and then looking at how it
deals with the various versions of the glibc affinity APIs (AFAIK there were 3
different ones).  Also, make sure you handle > 1024 CPUs properly.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[bviyer at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #4 from Balaji V. Iyer <bviyer at gmail dot com> ---
Hi Jakub,
    __cpu_set_t_defined is defined in /usr/include/bits/sched.h in my SUSE
machine and in Ubuntu machine. 

Thanks,

Balaji V. Iyer.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[PHHargrove at lbl dot gov]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #5 from Paul H. Hargrove <PHHargrove at lbl dot gov> ---
The patch in comment #2 allows the build to complete without the need to
--disable-libcilkrts.  However, I agree with Jakub (comment #3) that use of a
glibc internal macros is a poor idea and a configure-time check is prefered. 
Just because __cpu_set_t_defined exists today doesn't mean it will continue to
exist in later glibc revisions.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[barry.m.tannenbaum at intel dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

Barry Tannenbaum <barry.m.tannenbaum at intel dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |barry.m.tannenbaum at intel dot co
                   |                            |m

--- Comment #6 from Barry Tannenbaum <barry.m.tannenbaum at intel dot com> ---
Hi.  I'm one of the Cilk runtime developers.  Balaji asked me to help him with
this since it's a Cilk runtime issue.

I don't understand the reluctance to check the defintion of
__cpu_set_t_defined, since it's defined right there in
/usr/include/bits/sched.h on my Ubuntu system.  Is the problem that it's not a
"top-level" include file?

How about if I check the macros defined in /usr/include/sched.h.  I could
easily check for a defintion of CPU_SETSIZE, for example.

    - Barry

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Not everything in glibc headers is meant for use by other packages, some
symbols in there are just implementation details that can change any time.
The affinity support is complicated, as it has changed several times in the
history.  There are glibc 2.3.3 (2003-ish) versions of the APIs without size
parameters and 2.3.4 (2004-ish) versions of the APIs with size parameter, and
the macros changed several times, with the last API changes in glibc 2.7
(2007-ish).
So, ideally you want to grab glibc 2.3.2, glibc 2.3.3, glibc 2.3.4, glibc 2.5,
glibc 2.6, glibc 2.7 headers and test against all those.
You want to support more than 1024 CPUs in the masks too, at least for building
with sufficiently new glibc, handle the case when there is no affinity support
in glibc at all (or there is, but it fails at runtime because e.g. kernel is
too old), and depending on what versions of headers you want to handle (there
are still long time support distros with e.g. glibc 2.5) use either the non-_S
or _S macros.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[PHHargrove at lbl dot gov]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #8 from Paul H. Hargrove <PHHargrove at lbl dot gov> ---
(In reply to Barry Tannenbaum from comment #6)
> I don't understand the reluctance to check the defintion of
> __cpu_set_t_defined, since it's defined right there in
> /usr/include/bits/sched.h on my Ubuntu system.  Is the problem that it's not
> a "top-level" include file?


As with must symbols starting with "__", this one is an implementation detail
internal to glibc.  It may or may not have existed in earlier versions and
could be removed or replaced in future versions.

The "it's ok because it exists on MY system" defense is entirely inappropriate
for a portable software project. 

Beyond just the issue of using an internal symbol, Jakub rightly points out
that there are (at least) three revisions of the affinity API.  Use of symbol
__cpu_set_t_defined only distinguishes between the no-support case (as on Red
Hat 8) and the other three.  It does not distinguish AMONG the three other
cases.

In comment #3 Jakub points out that libgomp already has configure macros for
dealing with the affinity stuff.  Adopting/adapting those is probably the best
approach to the problem.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[bviyer at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #9 from Balaji V. Iyer <bviyer at gmail dot com> ---
Created attachment 31847
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31847&action=edit
Possible fix

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[bviyer at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #10 from Balaji V. Iyer <bviyer at gmail dot com> ---
Hi Paul and Jakub,
    I have attached a patch above (called "Possible Fix"). Does it work for
you?

Thanks,

Balaji V. Iyer.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[PHHargrove at lbl dot gov]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #12 from Paul H. Hargrove <PHHargrove at lbl dot gov> ---
Balaji,

Reading the patch it looks sufficient.
I will test when I have a chance, but cannot guess how soon that may be,

-Paul

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Why the aclocal.m4 removals?
Also, while the rest of the HAVE_PTHREAD_AFFINITY_NP addition is one desirable
step, I've said that it is not sufficient (but of course can be done
incrementally).
Why the source uses sched_getaffinity + gettid when it can use
pthread_getaffinity_np which will do the same thing and be less Linux OS
details dependent?  The code as is will not handle > 1024 CPU systems, see
PR57298.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[barry.m.tannenbaum at intel dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #14 from Barry Tannenbaum <barry.m.tannenbaum at intel dot com> ---
The original author of the code is no longer working on the Cilk project, so I
can't say why it's using sched_getaffinity + gettid instead the pthread
routine.  But I'll modify it to use the pthread routine and investigate support
for >1024 cores.

Is there a machine we can use to test that?

   - Barry

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The reporter of PR57298 supposedly has or had, I don't.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[bviyer at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #16 from Balaji V. Iyer <bviyer at gmail dot com> ---
Hi Jakub,
    Honestly, I don't know why the changes were done in aclocal.m4. I added the
change to configure.ac and then did the following commands:

autoreconf --force --verbose
automake --add-missing --verbose


Is there anything else (or any other flags) that I should do (or add)?

Thanks,

Balaji V. Iyer.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[law at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #17 from Jeffrey A. Law <law at redhat dot com> ---
If there's an interest, I could probably create a kickstart for a RHL8 VM
suitable for building GCC 4.9.   But it's a 10+ year old platform of marginal
value at best.

I'd be much more concerned about using cpu_set_t instead of the dynamically
sizing variants.  As Jakub noted cpu_set_t doesn't work with > 1024 processors
and it's been the source of some cross-distro problems as well.  If at all
possible we need to be moving away from using cpu_set_t to the newer APIs.

My recommendation would be to address the RHL8 problem (no cpu_set_t at all) as
efficiently as possible without burning huge amount of time on it.   Then a
separate BZ to track issues around moving away from cpu_set_t to the more
modern interfaces.

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[bviyer at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

--- Comment #18 from bviyer at gcc dot gnu.org ---
Author: bviyer
Date: Mon Jan 20 17:49:22 2014
New Revision: 206846

URL: http://gcc.gnu.org/viewcvs?rev=206846&root=gcc&view=rev
Log:
Fix for PR other/58996.
+2014-01-20  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+
+       PR other/58996
+       * configure.ac: Added a check for pthread affinity support.
+       * runtime/os-unix.c: Likewise.
+       * configure: Regenerate.
+


Modified:
    trunk/libcilkrts/ChangeLog
    trunk/libcilkrts/configure
    trunk/libcilkrts/configure.ac
    trunk/libcilkrts/runtime/os-unix.c

[Bug other/58996] [4.9 regression] build failure in libcilkrts

[law at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58996

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #19 from Jeffrey A. Law <law at redhat dot com> ---
Fixed by Balaji's trunk commit.