Closed Bug 1372890 Opened 2 years ago Closed 2 years ago

Define PRInt8 as 'char' on Solaris, when compiling as C++, to ensure it matches type int8_t

Categories

(NSPR :: NSPR, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: petr.sumbera, Assigned: petr.sumbera)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; SunOS i86pc; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20170518232530

Steps to reproduce:

During build on Solaris I got:

gmake[1]: Entering directory '/var/tmp/firefox/obj-x86_64-pc-solaris2.12/xpcom/tests'
/usr/bin/g++ -std=gnu++11 -o TestPRIntN.o -c -I/var/tmp/firefox/obj-x86_64-pc-solaris2.12/dist/stl_wrappers -I/var/tmp/firefox/obj-x86_64-pc-solaris2.12/dist/system_wrappers -include /var/tmp/firefox/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -I/var/tmp/firefox/xpcom/tests -I/var/tmp/firefox/obj-x86_64-pc-solaris2.12/xpcom/tests -I/var/tmp/firefox/xpcom/ds -I/var/tmp/firefox/obj-x86_64-pc-solaris2.12/dist/include  -I/var/tmp/firefox/obj-x86_64-pc-solaris2.12/dist/include/nspr -I/var/tmp/firefox/obj-x86_64-pc-solaris2.12/dist/include/nss       -fPIC  -DMOZILLA_CLIENT -include /var/tmp/firefox/obj-x86_64-pc-solaris2.12/mozilla-config.h -MD -MP -MF .deps/TestPRIntN.o.pp  -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++14-compat -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe  -g -O -fno-omit-frame-pointer     /var/tmp/firefox/xpcom/tests/TestPRIntN.cpp
/var/tmp/firefox/xpcom/tests/TestPRIntN.cpp: In function 'int main()':
/var/tmp/firefox/xpcom/tests/TestPRIntN.cpp:30:35: error: invalid conversion from 'int8_t* {aka char*}' to 'PRInt8* {aka signed char*}' [-fpermissive]
   ClearNSPRIntTypes(&w, &x, &y, &z);
                                   ^
/var/tmp/firefox/xpcom/tests/TestPRIntN.cpp:13:1: note:   initializing argument 1 of 'void ClearNSPRIntTypes(PRInt8*, PRInt16*, PRInt32*, PRInt64*)'
 ClearNSPRIntTypes(PRInt8 *a, PRInt16 *b, PRInt32 *c, PRInt64 *d)
 ^
/var/tmp/firefox/xpcom/tests/TestPRIntN.cpp:31:34: error: invalid conversion from 'PRInt8* {aka signed char*}' to 'int8_t* {aka char*}' [-fpermissive]
   ClearStdIntTypes(&a, &b, &c, &d);
                                  ^
/var/tmp/firefox/xpcom/tests/TestPRIntN.cpp:19:1: note:   initializing argument 1 of 'void ClearStdIntTypes(int8_t*, int16_t*, int32_t*, int64_t*)'
 ClearStdIntTypes(int8_t *w, int16_t *x, int32_t *y, int64_t *z)
 ^
gmake[1]: *** [/var/tmp/firefox/config/rules.mk:1061: TestPRIntN.o] Error 1
gmake[1]: Leaving directory '/var/tmp/firefox/obj-x86_64-pc-solaris2.12/xpcom/tests'
$ cat mytest.c
#include <stdint.h>
typedef signed char PRInt8;

static void
ClearNSPRIntTypes(PRInt8 *a)
{ 
  *a = 0;
}

static void
ClearStdIntTypes(int8_t *w)
{ 
  *w = 0;
}

int
main()
{ 
  PRInt8 a;
  int8_t w;

  ClearNSPRIntTypes(&w);
  ClearStdIntTypes(&a);
  return 0;
}
 	

$ g++ -c mytest.c
mytest.c: In function 'int main()':
mytest.c:22:23: error: invalid conversion from 'int8_t* {aka char*}' to 'PRInt8* {aka signed char*}' [-fpermissive]
   ClearNSPRIntTypes(&w);
                       ^
mytest.c:5:1: note:   initializing argument 1 of 'void ClearNSPRIntTypes(PRInt8*)'
 ClearNSPRIntTypes(PRInt8 *a)
 ^
mytest.c:23:22: error: invalid conversion from 'PRInt8* {aka signed char*}' to 'int8_t* {aka char*}' [-fpermissive]
   ClearStdIntTypes(&a);
                      ^
mytest.c:11:1: note:   initializing argument 1 of 'void ClearStdIntTypes(int8_t*)'
 ClearStdIntTypes(int8_t *w)
 ^
$ gcc -c mytest.c
$
$ g++ -E mytest.c | grep typedef | grep int8_t
typedef char int8_t;
typedef unsigned char uint8_t;

On Linux:

$ g++ -E mytest.c | grep typedef | grep int8_t
typedef signed char int8_t;
typedef unsigned char uint8_t;
Attached patch Bug1372890.patchSplinter Review
Attachment #8877603 - Flags: review?(jld)
Component: Untriaged → XPCOM
Product: Firefox → Core
Component: XPCOM → NSPR
Product: Core → NSPR
Version: Trunk → other
Comment on attachment 8877603 [details] [diff] [review]
Bug1372890.patch

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

Looks good to me, but I'd like to have an NSPR reviewer look at it.
Attachment #8877603 - Flags: review?(ted)
Attachment #8877603 - Flags: review?(jld)
Attachment #8877603 - Flags: review+
Comment on attachment 8877603 [details] [diff] [review]
Bug1372890.patch

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

Sorry, I resigned from NSPR ownership. I'll forward this to kaie.
Attachment #8877603 - Flags: review?(ted) → review?(kaie)
You are using GNU g++ on Solaris, and it doesn't support "signed char" ?

Which version are you using?

Are you sure it should be changed for all compiler versions on Solaris?
Should the define possibly be active only for old compiler versions?
I'm using gcc 5.4 on development version of Solaris. But the same is on Solaris 10 with gcc 3.4 and with Oracle Studio compiler.

Solaris does support signed char. It's just that Solaris headers defines int8_t as char (char is signed).
(In reply to Petr Sumbera from comment #6)
> It's just that Solaris headers defines
> int8_t as char (char is signed).

If char is signed, why does the compiler complain that conversion from "char*" to "signed char*" is invalid?
Assignee: nobody → petr.sumbera
Flags: needinfo?(petr.sumbera)
GCC seems to treat `char` as different from either `signed char` or `unsigned char` for the purpose of typechecking, even though it's equivalent to one of the two.  For example, this:

   char *cp;
   signed char *scp = cp;
   unsigned char *ucp = cp;

gives me -Wpointer-sign warnings ("pointer targets in initialization differ in signedness") for both of the conversions.  I don't know the rationale but I'd guess that it's meant to try to catch nonportable code that assumes a particular default char signedness in this way.
My Solaris and Ubuntu have both signed char:

root@ubuntu:~# cat signed.c
#include <limits.h>
#include <stdio.h>
int main() {printf("CHAR_MIN=%d\n", CHAR_MIN); return 0;}
root@ubuntu:~# g++ -o signed signed.c
root@ubuntu:~# ./signed
CHAR_MIN=-128

And if I define int8_t to be char (like Solaris headers do) on Linux. I get the same error I have on Solaris:

root@ubuntu:~# cat mytest2.c
typedef char int8_t;
typedef signed char PRInt8;

static void
ClearNSPRIntTypes(PRInt8 *a)
{
  *a = 0;
}

static void
ClearStdIntTypes(int8_t *w)
{
  *w = 0;
}

int
main()
{
  PRInt8 a;
  int8_t w;

  ClearNSPRIntTypes(&w);
  ClearStdIntTypes(&a);
  return 0;
}
root@ubuntu:~# g++ -c mytest2.c
mytest2.c: In function ▒int main()▒:
mytest2.c:22:23: error: invalid conversion from ▒int8_t* {aka char*}▒ to ▒PRInt8* {aka signed char*}▒ [-fpermissive]
   ClearNSPRIntTypes(&w);
                       ^
mytest2.c:5:1: note: initializing argument 1 of ▒void ClearNSPRIntTypes(PRInt8*)▒
 ClearNSPRIntTypes(PRInt8 *a)
 ^
mytest2.c:23:22: error: invalid conversion from ▒PRInt8* {aka signed char*}▒ to ▒int8_t* {aka char*}▒ [-fpermissive]
   ClearStdIntTypes(&a);
                      ^
mytest2.c:11:1: note: initializing argument 1 of ▒void ClearStdIntTypes(int8_t*)▒
 ClearStdIntTypes(int8_t *w)
 ^

I think that compiler probably doesn't know if char is signed or unsigned.
Flags: needinfo?(petr.sumbera)
Thanks for your research.

The type mismatch might be limited to C++ compilation, which seems to have been the reason why Wan-Teh added TestPRIntN.cpp to Firefox instead of NSPR, see 634793, and which is probably the reason why the defines in prtypes.h check for __cplusplus.

  #if (defined(HPUX) && defined(__cplusplus) \
        && !defined(__GNUC__) && __cplusplus < 199707L) \
    || (defined(SCO) && defined(__cplusplus) \
        && !defined(__GNUC__) && __cplusplus == 1L)

Wan-Teh also said in the bug:
  "Add the TestPRIntN.cpp test to xpcom/tests to make sure that
   PRInt{N} matches int{N}_t on all platforms."

I initially wasn't sure if this needs to be fixed at the NSPR level, but now that I have seen the intention, I agree that we need to fix NSPR to match the expectations on Solaris.

However, in the comment above the "typedef char PRInt8;" line, we should clarify why we must define it as "char", not "signed char" on Solaris.

I'll check in Petr's patch and add more comments.
Comment on attachment 8877603 [details] [diff] [review]
Bug1372890.patch

r=kaie but need more comments
Attachment #8877603 - Flags: review?(kaie) → review+
https://hg.mozilla.org/projects/nspr/rev/3b44b3fccb50
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Summary: xpcom/tests/TestPRIntN.cpp error: invalid conversion from 'int8_t* {aka char*}' to 'PRInt8* {aka signed char*}' (Solaris) → Define PRInt8 as 'char' on Solaris, when compiling as C++, to ensure it matches type int8_t
Target Milestone: --- → 4.16
(In reply to Kai Engert (:kaie:) from comment #10)
> The type mismatch might be limited to C++ compilation, which seems to have
> been the reason why Wan-Teh added TestPRIntN.cpp to Firefox instead of NSPR,
> see 634793, and which is probably the reason why the defines in prtypes.h
> check for __cplusplus.

It seems to be an error in C++ and a warning in C, so C code that's using -Wall -Werror would still have problems.
You need to log in before you can comment on or make changes to this bug.