This is an archive of the discontinued LLVM Phabricator instance.

[Orc] Specialize SerializationTraits on (un)signed char to fix SunOS
AbandonedPublic

Authored by mgorny on Oct 12 2016, 12:04 AM.

Details

Reviewers
lhames
Summary

Specialize the SerializationTraits on 'char', 'signed char'
and 'unsigned char' directly, rather than combining the first one with
'int8_t' and 'uint8_t'. While the latter types are commonly implemented
using 'signed char' and 'unsigned char', there is no guarantee that
a plain 'char' would not be used for their implementation.

This is exactly what happens on SunOS (OpenIndiana), where 'int8_t' is
implemented using 'char' (which is signed by default on the platform).
As a result, the specialization for 'int8_t' is equivalent to the one
for 'char' and causes the build to fail:

In file included from .../lib/ExecutionEngine/Orc/OrcRemoteTargetRPCAPI.cpp:10:
In file included from .../include/llvm/ExecutionEngine/Orc/OrcRemoteTargetRPCAPI.h:19:
.../include/llvm/ExecutionEngine/Orc/RPCByteChannel.h:161:7: error: redefinition of 'SerializationTraits<type-parameter-0-0, char, void>'
class SerializationTraits<ChannelT, char>
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.../psllvm/include/llvm/ExecutionEngine/Orc/RPCByteChannel.h:154:7: note: previous definition is here
class SerializationTraits<ChannelT, int8_t>
      ^
1 error generated.

Therefore, use 'signed char' and 'unsigned char' directly, since both
those types are guaranteed to be distinct from 'char'. They should also
be of the same size as int8_t/uint8_t on all platforms LLVM is being
built on.

Diff Detail

Event Timeline

mgorny updated this revision to Diff 74330.Oct 12 2016, 12:04 AM
mgorny retitled this revision from to [Orc] Specialize SerializationTraits on (un)signed char to fix SunOS.
mgorny updated this object.
mgorny added a reviewer: lhames.
mgorny added a subscriber: llvm-commits.
lhames edited edge metadata.Oct 18 2016, 11:44 AM

Hi Michał,

That's interesting. I think I ran into almost the opposite problem while working on an update to this code: leaving out a specialization for char caused a 'missing specialization' error.

Let me look into this: I suspect we'll actually want to use SFINAE tricks to enable/disable one of the specializations.

Cheers,
Lang.

Hi Michał,

That's interesting. I think I ran into almost the opposite problem while working on an update to this code: leaving out a specialization for char caused a 'missing specialization' error.

Let me look into this: I suspect we'll actually want to use SFINAE tricks to enable/disable one of the specializations.

That was my first thought as well. However, my C++ skills were not enough to be able to get a working SFINAE for this problem. So I went for the simplest alternative, that is specializing all three possible char variants.

mgorny abandoned this revision.Nov 12 2016, 2:46 PM

This seems no longer necessary after rL286620. Thanks!