Re: Fix D compilation on Solaris

[Rainer Orth <ro at CeBiTec dot Uni-Bielefeld dot DE>]

Hi Iain,

> On Wed, 31 Oct 2018 at 10:43, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote:
>>
>> Hi Iain,
>>
>> >> My first suspect here would be 'struct UnionExp', see d/dmd/expression.h
>> >>
>> >> Upstream dmd use a poor man's alignment, from what I recall to be
>> >> compatible with the dmc compiler.
>> >>
>> >>         // Ensure that the union is suitably aligned.
>> >>         real_t for_alignment_only;
>> >>
>> >> What happens if you were to replace that with marking the type as
>> >> __attribute__ ((aligned (8))) ?
>> >
>> > thanks for the suggestion: this worked just fine.  After a couple more
>> > libphobos adjustments (described below), I was able to finish the build
>> > on both sparc-sun-solaris2.11 and i386-pc-solaris2.11.
>> >
>> > The link tests still all fail as before, but sparc and x86 are now on
>> > par here :-)
>>
>> and now with the updated patch ;-)
>>
>
> Thanks, the front-end and library parts should be posted upstream.
>
> Mapping would be:
> - d/dmd: https://github.com/dlang/dmd/tree/dmd-cxx
> - libdruntime/core: https://github.com/dlang/druntime
> - libphobos/src/std: https://github.com/dlang/phobos
>
> I can take care of this, then backport/merge it down here.

that would be great.  I'd like to avoid becoming involved in the
procedures of too many upstream projects if possible.  The way Ian
handles my Solaris Go changes is very convenient for me ;-)

> As for the patch itself:
>
>> --- a/gcc/config/default-d.c
>> +++ b/gcc/config/default-d.c
>> @@ -18,6 +18,7 @@ along with GCC; see the file COPYING3.
>>  #include "config.h"
>>  #include "system.h"
>>  #include "coretypes.h"
>> +#include "memmodel.h"
>>  #include "tm_d.h"
>>  #include "d/d-target.h"
>>  #include "d/d-target-def.h"
>
> Is this still required?  For sure it would cover non-glibc,
> non-solaris sparc targets though.

There are other *-protos.h files using enum memmodel beside sparc:
alpha, ia64, and tilegx.  This may or may not be a reason to keep the
include.

>> diff --git a/gcc/config/sol2-d.c b/gcc/config/sol2-d.c
>> new file mode 100644
>> --- /dev/null
>> +++ b/gcc/config/sol2-d.c
>
> [-- snip --]
>
>> +solaris_d_os_builtins (void)
>> +{
>> +  d_add_builtin_version ("Posix");
>> +  d_add_builtin_version ("Solaris"); \
>> +}
>> +
>
> I'll assume that backslash is a typo.
>
> You'll also need to add this target hook:
>
>     /* Implement TARGET_D_CRITSEC_SIZE for Solaris targets.  */
>
>     static unsigned
>     solaris_d_critsec_size (void)
>     {
>       /* This is the sizeof pthread_mutex_t.  */
>       return 24;
>     }
>
> I hope that pthread_mutex_t does not differ between x86 and SPARC.

I saw it in glibc-d.c, but initially thought it were Linux-only in some
way.  Fortunately, pthread_mutex_t is identical between sparc and x86,
32 and 64-bit on Solaris.  Added to the updated patch.

>> diff --git a/gcc/config/t-sol2 b/gcc/config/t-sol2
>> --- a/gcc/config/t-sol2
>> +++ b/gcc/config/t-sol2
>> @@ -16,7 +16,7 @@
>>  # along with GCC; see the file COPYING3.  If not see
>>  # <http://www.gnu.org/licenses/>.
>>
>> -# Solaris-specific format checking and pragmas
>> +# Solaris-specific format checking and pragmas.
>>  sol2-c.o: $(srcdir)/config/sol2-c.c
>>  $(COMPILE) $<
>>  $(POSTCOMPILE)
>
> Not sure what the policy is about mixing unrelated changes in a patch here.

In general, they are frowned upon ;-)  However, in this case for a
single-character comment change in code I maintain, I believe it would
be overkill to move it to a separate check-in.

>> diff --git a/libphobos/libdruntime/core/sys/posix/ucontext.d
>> b/libphobos/libdruntime/core/sys/posix/ucontext.d
>> --- a/libphobos/libdruntime/core/sys/posix/ucontext.d
>> +++ b/libphobos/libdruntime/core/sys/posix/ucontext.d
>
> [-- snip --]
>
>> + struct fq
>> + {
>> +     union FQu
>> +     {
>> + double whole;
>> + _fpq fpq;
>> +     };
>> + }
>
> Just an FYI, this won't do what I think you expect, 'struct fq' here
> would be an empty struct.  Better make this an anonymous union, I can
> see the same mistake done elsewhere.
>
> struct fq
> {
>     union
>     {
>         double whole;
>         _fpq fpq;
>     }
> }

Thanks, all instances fixed, I hope.

>> diff --git a/libphobos/libdruntime/core/thread.d b/libphobos/libdruntime/core/thread.d
>> --- a/libphobos/libdruntime/core/thread.d
>> +++ b/libphobos/libdruntime/core/thread.d
>> @@ -1547,7 +1547,7 @@ private:
>>
>>      version (Solaris)
>>      {
>> -        __gshared immutable bool m_isRTClass;
>> +        __gshared bool m_isRTClass;
>>      }
>>
>>  private:
>
> This is curious, I wonder when was the last time someone tested x86
> Solaris in upstream.  What was the compilation error?

I got

/vol/gcc/src/hg/trunk/local/libphobos/libdruntime/core/thread.d:989:21: error: cannot modify immutable expression m_isRTClass
  989 |                     m_isRTClass = true;
      |                     ^
/vol/gcc/src/hg/trunk/local/libphobos/libdruntime/core/thread.d:997:21: error: cannot modify immutable expression m_isRTClass
  997 |                     m_isRTClass = false;
      |                     ^

>> diff --git a/libphobos/libdruntime/rt/sections_solaris.d
>> b/libphobos/libdruntime/rt/sections_solaris.d
>> --- a/libphobos/libdruntime/rt/sections_solaris.d
>> +++ b/libphobos/libdruntime/rt/sections_solaris.d
>
> [-- snip --]
>
>>
>> +// interface for core.thread to inherit loaded libraries
>> +void* pinLoadedLibraries() nothrow @nogc
>> +{
>> +    return null;
>> +}
>> +
>
> I ran into this on OSX as well.  The problem lies elsewhere, and I'm
> going to fix this.  The shared library is being built with
> '-fversion=Shared', but this is only valid for targets that use
> rt/sections_elf_shared.d
>
> I suspect that Solaris/SPARC should be using the ELF sections support anyway.

Certainly, given that SysVr4 invented ELF and Solaris inherited it from
there.

> Apart from comments, the GCC parts look OK to me.  Unless you want to
> raise a pull request, I'll submit the library patches upstream later.

Excellent, thanks.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University