Last Comment Bug 747621 - Apples Clang 4.0 gives a few -Wc++11-narrowing build errors
: Apples Clang 4.0 gives a few -Wc++11-narrowing build errors
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: TB-mountain-lion-com
  Show dependency treegraph
 
Reported: 2012-04-21 03:11 PDT by Nomis101
Modified: 2012-07-18 18:24 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Error build log (65.86 KB, text/plain)
2012-04-21 03:11 PDT, Nomis101
no flags Details
WIP - change PRInt32 to PRUint32 (2.88 KB, patch)
2012-04-24 12:35 PDT, Nomis101
no flags Details | Diff | Splinter Review
Workaround - pass -Wno-c++11-narrowing and port Bug 727145 (3.74 KB, patch)
2012-04-24 12:39 PDT, Nomis101
no flags Details | Diff | Splinter Review
Patch (1.19 KB, patch)
2012-06-16 14:55 PDT, Nomis101
standard8: review-
Details | Diff | Splinter Review
Like this? (5.16 KB, patch)
2012-06-29 12:52 PDT, Nomis101
no flags Details | Diff | Splinter Review
Like this? v2 (2.60 KB, patch)
2012-06-29 16:20 PDT, Nomis101
no flags Details | Diff | Splinter Review
Better patch (2.78 KB, patch)
2012-06-30 05:09 PDT, Nomis101
standard8: review+
Details | Diff | Splinter Review

Description Nomis101 2012-04-21 03:11:10 PDT
Created attachment 617200 [details]
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] 2012-04-21 06:10:41 PDT
Nomis can you try to make patches to fix these ?
Comment 2 Nomis101 2012-04-21 10:48:31 PDT
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 2012-04-23 07:45:42 PDT
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 2012-04-23 12:18:16 PDT
But I do not see the warning in these two files mentioned (on gcc 4.7).
Comment 5 Nomis101 2012-04-23 12:39:06 PDT
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 2012-04-23 12:49:18 PDT
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 2012-04-23 13:51:35 PDT
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 2012-04-23 13:57:42 PDT
I said the 'code' variable.
Comment 9 Nomis101 2012-04-24 12:33:10 PDT
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 2012-04-24 12:35:01 PDT
Created attachment 617991 [details] [diff] [review]
WIP - change PRInt32 to PRUint32

This fixes the errors in nsDirPrefs.cpp for me.
Comment 11 Nomis101 2012-04-24 12:39:02 PDT
Created attachment 617995 [details] [diff] [review]
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] 2012-05-14 04:00:33 PDT
Nomis are these patches complete ?
Comment 13 Nomis101 2012-05-14 10:54:20 PDT
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 2012-06-16 14:55:54 PDT
Created attachment 633846 [details] [diff] [review]
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) (Away until Aug 29) 2012-06-23 23:31:36 PDT
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) 2012-06-29 03:26:48 PDT
(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) 2012-06-29 03:27:55 PDT
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 2012-06-29 12:28:28 PDT
(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 2012-06-29 12:52:55 PDT
Created attachment 637976 [details] [diff] [review]
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) 2012-06-29 13:59:21 PDT
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 2012-06-29 16:20:35 PDT
Created attachment 638049 [details] [diff] [review]
Like this? v2

OK, this one also builds (with clang).
Comment 22 Nomis101 2012-06-30 05:09:50 PDT
Created attachment 638099 [details] [diff] [review]
Better patch

Now with a proper header :)
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-07-18 18:24:18 PDT
https://hg.mozilla.org/comm-central/rev/e7508a6c7dca

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

Note You need to log in before you can comment on or make changes to this bug.