747621 - Apples Clang 4.0 gives a few -Wc++11-narrowing build errors

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Nomis101]

Attachment: Error build log

Yesterday I've tried to build Thunderbird with Apples Clang 4.0. This produces a lot of errors in the kind of:

/Volumes/Developer/comm-central/mailnews/addrbook/src/nsDirPrefs.cpp:404:9: error: 
      case value evaluates to 2147483649, which cannot be narrowed to type
      'PRInt32' (aka 'int') [-Wc++11-narrowing]
   case DIR_POS_DELETE:
        ^

and


/Volumes/Developer/comm-central/mailnews/compose/src/nsSmtpProtocol.cpp:139:12: error: 
      case value evaluates to 2153066780, which cannot be narrowed to type 'int'
      [-Wc++11-narrowing]
      case NS_ERROR_SMTP_GREETING:
           ^

I've attached the full error log. It's build with the 10.7 SDK. I get no errors in Gecko.
Apple clang version 4.0 (tags/Apple/clang-421.0.11) (based on LLVM 3.1svn)

Comment 1

[Ludovic Hirlimann [:Usul]]

Nomis can you try to make patches to fix these ?

Comment 2

[Nomis101]

Maybe. I'm currently trying to workaround this, because I'm not sure how to fix it. But if somebody has an idea how to fix this, I can try it and make a patch.
I've asked :espindola about that and he told me that something like this can happen if build in c++11, because there are more restrictions on narrowing. If build in c++03, this is just a warning and a workaround would be to pass -Wno-c++11-narrowing. So I've tried building with -Wno-c++11-narrowing, and this indeed repressed the errors. But sadly it also produced new errors.

Comment 3

[:aceman]

I have seen tons of these warnings (in Core) too when I just switched to gcc 4.7. Only that it is a warning (not error) so the build succeeds.

I am not sure we can change the values of the error constants to be less then max value of PRInt32. Maybe we should make the variables PRUint32 ('position' and 'code').

Comment 4

[:aceman]

But I do not see the warning in these two files mentioned (on gcc 4.7).

Comment 5

[Nomis101]

As I read, Clang is a lot stricter as C++ compiler in warnings and errors than gcc, maybe thats the reason you do not see this with gcc. You mean changing the PRInt32 to PRUint32 could help?

Comment 6

[:aceman]

Yes, can you try that? Of course, try to check if those 2 variables do not take negative values at some other place.

Comment 7

[Nomis101]

OK, it seems PRUint32 works at least for nsDirPrefs.cpp, I don't see an error there anymore (and no new error). Do you think the same will also work for nsSmtpProtocol.cpp? There are a lot of PRInt32 and I'm unsure which of them I should change.

Comment 8

[:aceman]

I said the 'code' variable.

Comment 9

[Nomis101]

Until now I was not able to do the same for nsSmtpProtocol.cpp, it only produces me new errors. I will still try to get this work with PRUint32. In the meantime I will attach the WIP patch I currently have for nsDirPrefs.cpp. I also was able to make a workaround patch by passing -Wno-c++11-narrowing, I will also attach this.

Comment 10

[Nomis101]

Attachment: WIP - change PRInt32 to PRUint32

This fixes the errors in nsDirPrefs.cpp for me.

Comment 11

[Nomis101]

Attachment: Workaround - pass -Wno-c++11-narrowing and port Bug 727145

This patch lets me build Thunderbird with Apples Clang 4.0. It passes -Wno-c++11-narrowing to the compiler and ports Bug 727145. The port is neccessary to prevent errors like:

/In file included from
/Volumes/Developer/comm-central/mailnews/compose/src/nsMsgCompose.cpp:47:
In file included from ../../../mozilla/dist/include/nsIScriptContext.h:46:
In file included from ../../../mozilla/dist/include/jsfriendapi.h:43:
In file included from ../../../mozilla/dist/include/jsclass.h:48:
../../../mozilla/dist/include/jsapi.h:2149:1: error: 'JS_GetNaNValue' has
      C-linkage specified, but returns user-defined type 'jsval'
      (aka 'JS::Value') which is incompatible with C
      [-Werror,-Wreturn-type-c-linkage]
JS_GetNaNValue(JSContext *cx);
^

(:espíndola pointed me to this)

Comment 12

[Ludovic Hirlimann [:Usul]]

Nomis are these patches complete ?

Comment 13

[Nomis101]

The patch for nsDirPrefs.cpp is complete, but I was not able to fix the error for nsSmtpProtocol.cpp. The workaround patch works for building TB, but TB is build with this patch a bit crashy (don't know why). I currently switched back to Apples Clang 3.1

Comment 14

[Nomis101]

Attachment: Patch

With this patch and the patch from Bug 764469 I can build TB with Apples Clang 4.0. And it also seems to not interfere with older versions of Apples Clang. Tested with Apples Clang 4.0 on 10.8 and Apples Clang 3.1 on 10.7.4.

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 633846 [details] [diff] [review]
Patch

I won't get to this anytime soon, and I'm primarily a windows dev and don't have clang installed.

Comment 16

[Mark Banner (:standard8)]

(In reply to Nomis101 from comment #0)
> /Volumes/Developer/comm-central/mailnews/addrbook/src/nsDirPrefs.cpp:404:9:
> error: 
>       case value evaluates to 2147483649, which cannot be narrowed to type
>       'PRInt32' (aka 'int') [-Wc++11-narrowing]
>    case DIR_POS_DELETE:
>         ^

What happens if you simply replace DIR_POS_DELETE with -2 and DIR_POS_APPEND with -1 ? I believe that's what is trying to be represented, and I think clang is evaluating them as unsigned with more than 32 bits, when we do really want them as a 32 bit signed.

> /Volumes/Developer/comm-central/mailnews/compose/src/nsSmtpProtocol.cpp:139:
> 12: error: 
>       case value evaluates to 2153066780, which cannot be narrowed to type
> 'int'
>       [-Wc++11-narrowing]
>       case NS_ERROR_SMTP_GREETING:

The problem here is that NS_ERROR_SMTP_GREETING (and co) is actually a macro, and ends up using NS_ERROR_GENERATE_FAILURE, which as you'll see is a nsresult type. nsresult is an unsigned int, so this code is assuming its possible to get those values squeezed in. Generally because both are 32 bits, there's no long-term errors, but obviously compilers are being upgraded to warn against these issues.

I think the correct solution is to change the "int code" parameter to nsExplainErrorDetails to "nsresult code" and adapt any callers as necessary. Then they will all use the correct types.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 633846 [details] [diff] [review]
Patch

I don't think this is quite right to do based on my comments above, if we did do it I'd want to see it in mozilla-central first.

Comment 18

[Nomis101]

(In reply to Mark Banner (:standard8) from comment #16)
> (In reply to Nomis101 from comment #0)
> > /Volumes/Developer/comm-central/mailnews/addrbook/src/nsDirPrefs.cpp:404:9:
> > error: 
> >       case value evaluates to 2147483649, which cannot be narrowed to type
> >       'PRInt32' (aka 'int') [-Wc++11-narrowing]
> >    case DIR_POS_DELETE:
> >         ^
> 
> What happens if you simply replace DIR_POS_DELETE with -2 and DIR_POS_APPEND
> with -1 ?
Yes, this works.


> I think the correct solution is to change the "int code" parameter to
> nsExplainErrorDetails to "nsresult code" and adapt any callers as necessary.
> Then they will all use the correct types.
Hm, currently I'm not sure how to do that. I try to find out how to fix this this way.

Comment 19

[Nomis101]

Attachment: Like this?

Do you mean like this? This builds now for me and the build seems stable and doesn't crash. This was so surprisingly easy...

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 637976 [details] [diff] [review]
Like this?

>-#define DIR_POS_APPEND                     0x80000000
>-#define DIR_POS_DELETE                     0x80000001

I think I'd prefer these to just be changed to const PRInt32 ...

>-nsresult nsExplainErrorDetails(nsISmtpUrl * aSmtpUrl, int code, ...)
>+nsresult nsExplainErrorDetails(nsISmtpUrl * aSmtpUrl, nsresult code, ...)

Yep its pretty much that simple, treat the code as a proper nsresult ;-) For consistencies sake, it'd be good to change the "int errorcode" to "nsresult errorcode" here as well:

http://hg.mozilla.org/comm-central/annotate/9128c033ee05/mailnews/compose/src/nsSmtpProtocol.cpp#l1432

as that's calling nsExplainErrorDetails that you're altering. All the other instances where nsExplainErrorDetails is called use the constants, and hence don't need changing.

Comment 21

[Nomis101]

Attachment: Like this? v2

OK, this one also builds (with clang).

Comment 22

[Nomis101]

Attachment: Better patch

Now with a proper header :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/e7508a6c7dca

My, what a descriptive commit message. Please provide something a bit more...descriptive...next time?