<html><body>
<p><font size="2" face="sans-serif">Hi</font><br>
<br>
<font size="2" face="sans-serif">This worked well. Below a new version of the patch with this new modification.</font><br>
<font size="2" face="sans-serif">Do you have any other objections to this?</font><br>
<br>
<br>
<font size="2" face="sans-serif">Thanks!</font><br>
<br>
<font size="2" face="sans-serif">-- Carlo</font><br>
<br>
<font size="2" face="sans-serif">diff --git a/runtime/src/kmp_error.c b/runtime/src/kmp_error.c</font><br>
<font size="2" face="sans-serif">index b76c109..825157a 100644</font><br>
<font size="2" face="sans-serif">--- a/runtime/src/kmp_error.c</font><br>
<font size="2" face="sans-serif">+++ b/runtime/src/kmp_error.c</font><br>
<font size="2" face="sans-serif">@@ -111,7 +111,7 @@ __kmp_expand_cons_stack( int gtid, struct cons_header *p )</font><br>
<font size="2" face="sans-serif"> // NOTE: Function returns allocated memory, caller must free it!</font><br>
<font size="2" face="sans-serif"> static char const *</font><br>
<font size="2" face="sans-serif"> __kmp_pragma(</font><br>
<font size="2" face="sans-serif">- enum cons_type ct,</font><br>
<font size="2" face="sans-serif">+ int ct,</font><br>
<font size="2" face="sans-serif"> ident_t const * ident</font><br>
<font size="2" face="sans-serif"> ) {</font><br>
<font size="2" face="sans-serif"> char const * cons = NULL; // Construct name.</font><br>
<font size="2" face="sans-serif">@@ -121,7 +121,7 @@ __kmp_pragma(</font><br>
<font size="2" face="sans-serif"> kmp_str_buf_t buffer;</font><br>
<font size="2" face="sans-serif"> kmp_msg_t prgm;</font><br>
<font size="2" face="sans-serif"> __kmp_str_buf_init( & buffer );</font><br>
<font size="2" face="sans-serif">- if ( 0 < ct && ct <= cons_text_c_num ) {;</font><br>
<font size="2" face="sans-serif">+ if ( 0 < ct && ct <= cons_text_c_num ) {</font><br>
<font size="2" face="sans-serif"> cons = cons_text_c[ ct ];</font><br>
<font size="2" face="sans-serif"> } else {</font><br>
<font size="2" face="sans-serif"> KMP_DEBUG_ASSERT( 0 );</font><br>
<font size="2" face="sans-serif">diff --git a/runtime/src/kmp_runtime.c b/runtime/src/kmp_runtime.c</font><br>
<font size="2" face="sans-serif">index 55b58ce..0a08993 100644</font><br>
<font size="2" face="sans-serif">--- a/runtime/src/kmp_runtime.c</font><br>
<font size="2" face="sans-serif">+++ b/runtime/src/kmp_runtime.c</font><br>
<font size="2" face="sans-serif">@@ -267,7 +267,8 @@ __kmp_check_stack_overlap( kmp_info_t *th )</font><br>
<font size="2" face="sans-serif"> }</font><br>
<br>
<font size="2" face="sans-serif"> /* No point in checking ubermaster threads since they use refinement and cannot overlap */</font><br>
<font size="2" face="sans-serif">- if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid = __kmp_gtid_from_thread( th )))</font><br>
<font size="2" face="sans-serif">+ gtid = __kmp_gtid_from_thread( th );</font><br>
<font size="2" face="sans-serif">+ if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid))</font><br>
<font size="2" face="sans-serif"> {</font><br>
<font size="2" face="sans-serif"> KA_TRACE(10,("__kmp_check_stack_overlap: performing extensive checking\n"));</font><br>
<font size="2" face="sans-serif"> if ( stack_beg == NULL ) {</font><br>
<br>
<br>
<br>
<img width="16" height="16" src="cid:1__=0ABBF76FDFC92E288f9e8a93df938@us.ibm.com" border="0" alt="Inactive hide details for "Cownie, James H" ---03/02/2015 11:27:36 AM---I do understand the casting solution, but I am not sure"><font size="2" color="#424282" face="sans-serif">"Cownie, James H" ---03/02/2015 11:27:36 AM---I do understand the casting solution, but I am not sure what function you are referring to in your l</font><br>
<br>
<font size="1" color="#5F5F5F" face="sans-serif">From: </font><font size="1" face="sans-serif">"Cownie, James H" <james.h.cownie@intel.com></font><br>
<font size="1" color="#5F5F5F" face="sans-serif">To: </font><font size="1" face="sans-serif">Carlo Bertolli/Watson/IBM@IBMUS</font><br>
<font size="1" color="#5F5F5F" face="sans-serif">Cc: </font><font size="1" face="sans-serif">Hal Finkel <hfinkel@anl.gov>, "openmp-dev@dcs-maillist2.engr.illinois.edu" <openmp-dev@dcs-maillist2.engr.illinois.edu></font><br>
<font size="1" color="#5F5F5F" face="sans-serif">Date: </font><font size="1" face="sans-serif">03/02/2015 11:27 AM</font><br>
<font size="1" color="#5F5F5F" face="sans-serif">Subject: </font><font size="1" face="sans-serif">RE: [Openmp-dev] Two warnings with clang 3.5 on ppc63le</font><br>
<hr width="100%" size="2" align="left" noshade style="color:#8091A5; "><br>
<br>
<br>
<a name="_MailEndCompose"></a><font size="2" face="Arial">I do understand the casting solution, but I am not sure what function you are referring to in your last message.<br>
Can you please explain?</font><font size="3" face="Times New Roman"><br>
</font><br>
<font size="2" color="#1F497D" face="Verdana">My proposal would be to change the interface to </font><br>
<font size="2" color="#1F497D" face="Verdana"> </font><br>
<font size="2" color="#1F497D" face="Lucida Console">static char const *</font><br>
<font size="2" color="#1F497D" face="Lucida Console">__kmp_pragma(</font><br>
<font size="2" color="#1F497D" face="Lucida Console"> enum cons_type ct,</font><br>
<font size="2" color="#1F497D" face="Lucida Console"> ident_t const * ident</font><br>
<font size="2" color="#1F497D" face="Lucida Console">) </font><br>
<font size="2" color="#1F497D" face="Verdana"> </font><br>
<font size="2" color="#1F497D" face="Verdana">Which is the function in question, I believe, to be </font><br>
<font size="2" color="#1F497D" face="Lucida Console"> </font><br>
<font size="2" color="#1F497D" face="Lucida Console">static char const *</font><br>
<font size="2" color="#1F497D" face="Lucida Console">__kmp_pragma(</font><br>
<font size="2" color="#1F497D" face="Lucida Console"> int ct,</font><br>
<font size="2" color="#1F497D" face="Lucida Console"> ident_t const * ident</font><br>
<font size="2" color="#1F497D" face="Lucida Console">) </font><br>
<font size="2" color="#1F497D" face="Verdana"> </font><br>
<font size="2" color="#1F497D" face="Verdana">At the call site, this should still be OK, because (IIRC) C allows a silent case from an enum to an int, the hope, then, is that since inside this function the compiler only knows that ct is an integer it won’t believe that its range is restricted, and therefore that the test is unnecessary. Of course, the compiler may still try to be cleverer than that, and to inline the function, in which case it will notice that the only arguments passed are of type </font><font size="2" color="#1F497D" face="Lucida Console">enum cons_type</font><font size="2" color="#1F497D" face="Verdana"> in which case we’ll be back where we started :-(.</font><br>
<font size="2" color="#1F497D" face="Verdana"> </font><br>
<font size="2" color="#1F497D" face="Verdana">The real issue here is that we’re trying to be very conservative in this code because we’re dealing with error messages so we know something is wrong and that might have been caused by random stores from buggy user code which could have corrupted the place we’re reading the enumeration from, but the compiler is working on the basis of optimizing correct code…</font><br>
<font size="2" color="#1F497D" face="Verdana"> </font><br>
<font size="2" color="#1F497D" face="Verdana">-- Jim<br>
<br>
James Cownie <james.h.cownie@intel.com><br>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)</font><br>
<font size="2" color="#1F497D" face="Verdana">Tel: +44 117 9071438</font><br>
<font size="2" color="#1F497D" face="Verdana"> </font><br>
<font size="2" face="Calibri"><b>From:</b></font><font size="2" face="Calibri"> Carlo Bertolli [</font><font size="2" face="Calibri"><a href="mailto:cbertol@us.ibm.com">mailto:cbertol@us.ibm.com</a></font><font size="2" face="Calibri">] </font><font size="2" face="Calibri"><b><br>
Sent:</b></font><font size="2" face="Calibri"> Monday, March 2, 2015 4:16 PM</font><font size="2" face="Calibri"><b><br>
To:</b></font><font size="2" face="Calibri"> Cownie, James H</font><font size="2" face="Calibri"><b><br>
Cc:</b></font><font size="2" face="Calibri"> Hal Finkel; openmp-dev@dcs-maillist2.engr.illinois.edu</font><font size="2" face="Calibri"><b><br>
Subject:</b></font><font size="2" face="Calibri"> RE: [Openmp-dev] Two warnings with clang 3.5 on ppc63le</font><br>
<font size="3" face="Times New Roman"> </font>
<p><font size="2" face="Arial">Hi Jim, Hal,</font><font size="3" face="Times New Roman"><br>
</font><font size="2" face="Arial"><br>
Thanks for explaining this.</font><font size="3" face="Times New Roman"><br>
</font><font size="2" face="Arial"><br>
I do understand the casting solution, but I am not sure what function you are referring to in your last message.<br>
Can you please explain?</font><font size="3" face="Times New Roman"><br>
<br>
</font><font size="2" face="Arial"><br>
Thanks</font><font size="3" face="Times New Roman"><br>
</font><font size="2" face="Arial"><br>
-- Carlo</font><font size="3" face="Times New Roman"><br>
<br>
<br>
<br>
</font><img src="cid:1__=0ABBF76FDFC92E288f9e8a93df938@us.ibm.com" width="16" height="16" alt="Inactive hide details for "Cownie, James H" ---03/02/2015 06:28:19 AM---> If you'd like to prevent this, you'll need to change "><font size="2" color="#424282" face="Arial">"Cownie, James H" ---03/02/2015 06:28:19 AM---> If you'd like to prevent this, you'll need to change the type of the variable to int > (or unsign</font><font size="3" face="Times New Roman"><br>
</font><font size="1" color="#5F5F5F" face="Arial"><br>
From: </font><font size="1" face="Arial">"Cownie, James H" <</font><a href="mailto:james.h.cownie@intel.com"><font size="1" color="#0000FF" face="Arial"><u>james.h.cownie@intel.com</u></font></a><font size="1" face="Arial">></font><font size="1" color="#5F5F5F" face="Arial"><br>
To: </font><font size="1" face="Arial">Hal Finkel <</font><a href="mailto:hfinkel@anl.gov"><font size="1" color="#0000FF" face="Arial"><u>hfinkel@anl.gov</u></font></a><font size="1" face="Arial">></font><font size="1" color="#5F5F5F" face="Arial"><br>
Cc: </font><font size="1" face="Arial">Carlo Bertolli/Watson/IBM@IBMUS, "</font><a href="mailto:openmp-dev@dcs-maillist2.engr.illinois.edu"><font size="1" color="#0000FF" face="Arial"><u>openmp-dev@dcs-maillist2.engr.illinois.edu</u></font></a><font size="1" face="Arial">" <</font><a href="mailto:openmp-dev@dcs-maillist2.engr.illinois.edu"><font size="1" color="#0000FF" face="Arial"><u>openmp-dev@dcs-maillist2.engr.illinois.edu</u></font></a><font size="1" face="Arial">></font><font size="1" color="#5F5F5F" face="Arial"><br>
Date: </font><font size="1" face="Arial">03/02/2015 06:28 AM</font><font size="1" color="#5F5F5F" face="Arial"><br>
Subject: </font><font size="1" face="Arial">RE: [Openmp-dev] Two warnings with clang 3.5 on ppc63le</font><br>
<hr width="100%" size="2" align="left" noshade><br>
<font size="3" face="Times New Roman"><br>
<br>
</font><font size="2" face="Courier New"><br>
> If you'd like to prevent this, you'll need to change the type of the variable to int <br>
> (or unsigned int, etc.). You might notice that in LLVM, for example, we have many 'enum' <br>
> variables that are typed as 'int' or 'unsigned' for a similar reason (if you want to use <br>
> an enum to name some possible integer values, but allow others, you can't actually use an <br>
> enum as the variable type).<br>
<br>
So the right answer, then, would be to change the formal argument of the function to be of <br>
type "int", rather than the enum type, and rely on the fact that enums are silently, automatically, cast<br>
to int.<br>
<br>
-- Jim<br>
<br>
James Cownie <</font><a href="mailto:james.h.cownie@intel.com"><font size="2" color="#0000FF" face="Courier New"><u>james.h.cownie@intel.com</u></font></a><font size="2" face="Courier New">><br>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)<br>
Tel: +44 117 9071438<br>
<br>
-----Original Message-----<br>
From: Hal Finkel [</font><a href="mailto:hfinkel@anl.gov"><font size="2" color="#0000FF" face="Courier New"><u>mailto:hfinkel@anl.gov</u></font></a><font size="2" face="Courier New">] <br>
Sent: Monday, March 2, 2015 11:15 AM<br>
To: Cownie, James H<br>
Cc: Carlo Bertolli; </font><a href="mailto:openmp-dev@dcs-maillist2.engr.illinois.edu"><font size="2" color="#0000FF" face="Courier New"><u>openmp-dev@dcs-maillist2.engr.illinois.edu</u></font></a><font size="2" face="Courier New"><br>
Subject: Re: [Openmp-dev] Two warnings with clang 3.5 on ppc63le<br>
<br>
----- Original Message -----<br>
> From: "James H Cownie" <</font><a href="mailto:james.h.cownie@intel.com"><font size="2" color="#0000FF" face="Courier New"><u>james.h.cownie@intel.com</u></font></a><font size="2" face="Courier New">><br>
> To: "Carlo Bertolli" <</font><a href="mailto:cbertol@us.ibm.com"><font size="2" color="#0000FF" face="Courier New"><u>cbertol@us.ibm.com</u></font></a><font size="2" face="Courier New">>, </font><a href="mailto:openmp-dev@dcs-maillist2.engr.illinois.edu"><font size="2" color="#0000FF" face="Courier New"><u>openmp-dev@dcs-maillist2.engr.illinois.edu</u></font></a><font size="2" face="Courier New"><br>
> Sent: Monday, March 2, 2015 4:28:27 AM<br>
> Subject: Re: [Openmp-dev] Two warnings with clang 3.5 on ppc63le<br>
> <br>
> 1.<br>
> /home/compteam/slave0/build_folder_omp/libiomp-src/runtime/src/kmp_error.c:124:23:<br>
> warning: comparison of constant 16 with expression of type 'enum<br>
> cons_type' is always true<br>
> [-Wtautological-constant-out-of-range-compare]<br>
> if ( 0 < ct && ct <= cons_text_c_num ) {;<br>
> ~~ ^ ~~~~~~~~~~~~~~~<br>
> <br>
> It looks like that there are no machines where const char * is more<br>
> that 64-bit. The second check can be removed safely - maybe the<br>
> author of this intended something different?<br>
> <br>
> I think you’re misunderstanding the point here. cons_text_c_num gives<br>
> us the number of elements in the table. The point so to ensure that<br>
> the value we’ve been passed is a correct index into the table. The<br>
> compiler is right that * if * the value passed is a correct member<br>
> of the enumeration it can’t be outside the range, but the check is<br>
> there precisely to ensure that the value passed in is in range<br>
> (because we can’t tell here that someone didn’t call this with<br>
> “(enum cons_type)1000”).<br>
> <br>
> So I think what we need here is a cast so that your compiler is<br>
> happy<br>
<br>
To be clear, the problem with the happiness of the compiler here is that, as written, the compiler is free to remove the check all together. Adding the cast may silence the warning, but the compiler's optimizer is still free to infer the range of the variable based on its original type. If you'd like to prevent this, you'll need to change the type of the variable to int (or unsigned int, etc.). You might notice that in LLVM, for example, we have many 'enum' variables that are typed as 'int' or 'unsigned' for a similar reason (if you want to use an enum to name some possible integer values, but allow others, you can't actually use an enum as the variable type).<br>
<br>
-Hal<br>
<br>
>, rather than removing the test.<br>
> <br>
> Something like<br>
> <br>
> if ( 0 < ct && ((int)ct) <= cons_text_c_num ) {;<br>
> <br>
> You could write the whole thing as<br>
> <br>
> if ( ((unsigned int)ct) <= cons_text_c_num ) {;<br>
> but that may be a little too clever for its own good, especially<br>
> since the performance of this code can’t matter.<br>
> <br>
> -- Jim<br>
> <br>
> James Cownie <</font><a href="mailto:james.h.cownie@intel.com"><font size="2" color="#0000FF" face="Courier New"><u>james.h.cownie@intel.com</u></font></a><font size="2" face="Courier New">><br>
> SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)<br>
> <br>
> Tel: +44 117 9071438<br>
> <br>
> <br>
> <br>
> <br>
> <br>
> From: </font><a href="mailto:openmp-dev-bounces@cs.uiuc.edu"><font size="2" color="#0000FF" face="Courier New"><u>openmp-dev-bounces@cs.uiuc.edu</u></font></a><font size="2" face="Courier New"><br>
> [</font><a href="mailto:openmp-dev-bounces@cs.uiuc.edu"><font size="2" color="#0000FF" face="Courier New"><u>mailto:openmp-dev-bounces@cs.uiuc.edu</u></font></a><font size="2" face="Courier New">] On Behalf Of Carlo Bertolli<br>
> Sent: Friday, February 27, 2015 10:57 PM<br>
> To: </font><a href="mailto:openmp-dev@dcs-maillist2.engr.illinois.edu"><font size="2" color="#0000FF" face="Courier New"><u>openmp-dev@dcs-maillist2.engr.illinois.edu</u></font></a><font size="2" face="Courier New"><br>
> Subject: [Openmp-dev] Two warnings with clang 3.5 on ppc63le<br>
> <br>
> <br>
> <br>
> Hi<br>
> <br>
> Using clang 3.5 on ppc64le, I obtained the following warnings.<br>
> <br>
> 1.<br>
> /home/compteam/slave0/build_folder_omp/libiomp-src/runtime/src/kmp_error.c:124:23:<br>
> warning: comparison of constant 16 with expression of type 'enum<br>
> cons_type' is always true<br>
> [-Wtautological-constant-out-of-range-compare]<br>
> if ( 0 < ct && ct <= cons_text_c_num ) {;<br>
> ~~ ^ ~~~~~~~~~~~~~~~<br>
> <br>
> It looks like that there are no machines where const char * is more<br>
> that 64-bit. The second check can be removed safely - maybe the<br>
> author of this intended something different?<br>
> <br>
> 2.<br>
> /home/compteam/slave0/build_folder_omp/libiomp-src/runtime/src/kmp_runtime.c:270:58:<br>
> warning: multiple unsequenced modifications to 'gtid'<br>
> [-Wunsequenced]<br>
> if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid =<br>
> __kmp_gtid_from_thread( th )))<br>
> ^<br>
> /home/compteam/slave0/build_folder_omp/libiomp-src/runtime/src/kmp.h:958:25:<br>
> note: expanded from macro 'KMP_UBER_GTID'<br>
> (__kmp_threads[(gtid)] == __kmp_root[(gtid)]->r.r_uber_thread)\<br>
> ^<br>
> <br>
> gtid here is modified multiple times. We can just move the assignment<br>
> out of the if and have only gtid as parameter of the macro.<br>
> <br>
> <br>
> Below a patch that fixes these problems. Please let me know your<br>
> thoughts on this.<br>
> <br>
> Thanks<br>
> <br>
> -- Carlo<br>
> <br>
> <br>
> <br>
> <br>
> diff --git a/runtime/src/kmp_error.c b/runtime/src/kmp_error.c<br>
> index b76c109..4274365 100644<br>
> --- a/runtime/src/kmp_error.c<br>
> +++ b/runtime/src/kmp_error.c<br>
> @@ -121,7 +121,7 @@ __kmp_pragma(<br>
> kmp_str_buf_t buffer;<br>
> kmp_msg_t prgm;<br>
> __kmp_str_buf_init( & buffer );<br>
> - if ( 0 < ct && ct <= cons_text_c_num ) {;<br>
> + if ( 0 < ct ) {<br>
> cons = cons_text_c[ ct ];<br>
> } else {<br>
> KMP_DEBUG_ASSERT( 0 );<br>
> diff --git a/runtime/src/kmp_runtime.c b/runtime/src/kmp_runtime.c<br>
> index 55b58ce..0a08993 100644<br>
> --- a/runtime/src/kmp_runtime.c<br>
> +++ b/runtime/src/kmp_runtime.c<br>
> @@ -267,7 +267,8 @@ __kmp_check_stack_overlap( kmp_info_t *th )<br>
> }<br>
> <br>
> /* No point in checking ubermaster threads since they use refinement<br>
> and cannot overlap */<br>
> - if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid =<br>
> __kmp_gtid_from_thread( th )))<br>
> + gtid = __kmp_gtid_from_thread( th );<br>
> + if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid))<br>
> {<br>
> KA_TRACE(10,("__kmp_check_stack_overlap: performing extensive<br>
> checking\n"));<br>
> if ( stack_beg == NULL ) {<br>
> <br>
> ---------------------------------------------------------------------<br>
> Intel Corporation (UK) Limited<br>
> Registered No. 1134945 (England)<br>
> Registered Office: Pipers Way, Swindon SN3 1RJ<br>
> VAT No: 860 2173 47<br>
> <br>
> This e-mail and any attachments may contain confidential material for<br>
> the sole use of the intended recipient(s). Any review or distribution<br>
> by others is strictly prohibited. If you are not the intended<br>
> recipient, please contact the sender and delete all copies.<br>
> _______________________________________________<br>
> Openmp-dev mailing list<br>
> </font><a href="mailto:Openmp-dev@dcs-maillist2.engr.illinois.edu"><font size="2" color="#0000FF" face="Courier New"><u>Openmp-dev@dcs-maillist2.engr.illinois.edu</u></font></a><font size="2" face="Courier New"><br>
> </font><a href="http://lists.cs.uiuc.edu/mailman/listinfo/openmp-dev"><font size="2" color="#0000FF" face="Courier New"><u>http://lists.cs.uiuc.edu/mailman/listinfo/openmp-dev</u></font></a><font size="2" face="Courier New"><br>
> <br>
<br>
-- <br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
---------------------------------------------------------------------<br>
Intel Corporation (UK) Limited<br>
Registered No. 1134945 (England)<br>
Registered Office: Pipers Way, Swindon SN3 1RJ<br>
VAT No: 860 2173 47<br>
<br>
This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.</font>
<p><font size="3" face="Times New Roman">---------------------------------------------------------------------<br>
Intel Corporation (UK) Limited<br>
Registered No. 1134945 (England)<br>
Registered Office: Pipers Way, Swindon SN3 1RJ<br>
VAT No: 860 2173 47</font>
<p><font size="3" face="Times New Roman">This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.</font>
<p></body></html>