[Openmp-dev] Two warnings with clang 3.5 on ppc63le
Carlo Bertolli
cbertol at us.ibm.com
Mon Mar 2 08:40:36 PST 2015
Hi
This worked well. Below a new version of the patch with this new
modification.
Do you have any other objections to this?
Thanks!
-- Carlo
diff --git a/runtime/src/kmp_error.c b/runtime/src/kmp_error.c
index b76c109..825157a 100644
--- a/runtime/src/kmp_error.c
+++ b/runtime/src/kmp_error.c
@@ -111,7 +111,7 @@ __kmp_expand_cons_stack( int gtid, struct cons_header
*p )
// NOTE: Function returns allocated memory, caller must free it!
static char const *
__kmp_pragma(
- enum cons_type ct,
+ int ct,
ident_t const * ident
) {
char const * cons = NULL; // Construct name.
@@ -121,7 +121,7 @@ __kmp_pragma(
kmp_str_buf_t buffer;
kmp_msg_t prgm;
__kmp_str_buf_init( & buffer );
- if ( 0 < ct && ct <= cons_text_c_num ) {;
+ if ( 0 < ct && ct <= cons_text_c_num ) {
cons = cons_text_c[ ct ];
} else {
KMP_DEBUG_ASSERT( 0 );
diff --git a/runtime/src/kmp_runtime.c b/runtime/src/kmp_runtime.c
index 55b58ce..0a08993 100644
--- a/runtime/src/kmp_runtime.c
+++ b/runtime/src/kmp_runtime.c
@@ -267,7 +267,8 @@ __kmp_check_stack_overlap( kmp_info_t *th )
}
/* No point in checking ubermaster threads since they use refinement
and cannot overlap */
- if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid =
__kmp_gtid_from_thread( th )))
+ gtid = __kmp_gtid_from_thread( th );
+ if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid))
{
KA_TRACE(10,("__kmp_check_stack_overlap: performing extensive
checking\n"));
if ( stack_beg == NULL ) {
From: "Cownie, James H" <james.h.cownie at intel.com>
To: Carlo Bertolli/Watson/IBM at IBMUS
Cc: Hal Finkel <hfinkel at anl.gov>,
"openmp-dev at dcs-maillist2.engr.illinois.edu"
<openmp-dev at dcs-maillist2.engr.illinois.edu>
Date: 03/02/2015 11:27 AM
Subject: RE: [Openmp-dev] Two warnings with clang 3.5 on ppc63le
I do understand the casting solution, but I am not sure what function you
are referring to in your last message.
Can you please explain?
My proposal would be to change the interface to
static char const *
__kmp_pragma(
enum cons_type ct,
ident_t const * ident
)
Which is the function in question, I believe, to be
static char const *
__kmp_pragma(
int ct,
ident_t const * ident
)
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 enum cons_type in which case we’ll be back where we
started :-(.
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…
-- Jim
James Cownie <james.h.cownie at intel.com>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: +44 117 9071438
From: Carlo Bertolli [mailto:cbertol at us.ibm.com]
Sent: Monday, March 2, 2015 4:16 PM
To: Cownie, James H
Cc: Hal Finkel; openmp-dev at dcs-maillist2.engr.illinois.edu
Subject: RE: [Openmp-dev] Two warnings with clang 3.5 on ppc63le
Hi Jim, Hal,
Thanks for explaining this.
I do understand the casting solution, but I am not sure what function you
are referring to in your last message.
Can you please explain?
Thanks
-- Carlo
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 "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
From: "Cownie, James H" <james.h.cownie at intel.com>
To: Hal Finkel <hfinkel at anl.gov>
Cc: Carlo Bertolli/Watson/IBM at IBMUS, "
openmp-dev at dcs-maillist2.engr.illinois.edu" <
openmp-dev at dcs-maillist2.engr.illinois.edu>
Date: 03/02/2015 06:28 AM
Subject: RE: [Openmp-dev] Two warnings with clang 3.5 on ppc63le
> 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).
So the right answer, then, would be to change the formal argument of the
function to be of
type "int", rather than the enum type, and rely on the fact that enums are
silently, automatically, cast
to int.
-- Jim
James Cownie <james.h.cownie at intel.com>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: +44 117 9071438
-----Original Message-----
From: Hal Finkel [mailto:hfinkel at anl.gov]
Sent: Monday, March 2, 2015 11:15 AM
To: Cownie, James H
Cc: Carlo Bertolli; openmp-dev at dcs-maillist2.engr.illinois.edu
Subject: Re: [Openmp-dev] Two warnings with clang 3.5 on ppc63le
----- Original Message -----
> From: "James H Cownie" <james.h.cownie at intel.com>
> To: "Carlo Bertolli" <cbertol at us.ibm.com>,
openmp-dev at dcs-maillist2.engr.illinois.edu
> Sent: Monday, March 2, 2015 4:28:27 AM
> Subject: Re: [Openmp-dev] Two warnings with clang 3.5 on ppc63le
>
> 1.
> /home/compteam/slave0/build_folder_omp/libiomp-src/runtime/src/kmp_error.c:124:23:
> warning: comparison of constant 16 with expression of type 'enum
> cons_type' is always true
> [-Wtautological-constant-out-of-range-compare]
> if ( 0 < ct && ct <= cons_text_c_num ) {;
> ~~ ^ ~~~~~~~~~~~~~~~
>
> It looks like that there are no machines where const char * is more
> that 64-bit. The second check can be removed safely - maybe the
> author of this intended something different?
>
> I think you’re misunderstanding the point here. cons_text_c_num gives
> us the number of elements in the table. The point so to ensure that
> the value we’ve been passed is a correct index into the table. The
> compiler is right that * if * the value passed is a correct member
> of the enumeration it can’t be outside the range, but the check is
> there precisely to ensure that the value passed in is in range
> (because we can’t tell here that someone didn’t call this with
> “(enum cons_type)1000”).
>
> So I think what we need here is a cast so that your compiler is
> happy
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).
-Hal
>, rather than removing the test.
>
> Something like
>
> if ( 0 < ct && ((int)ct) <= cons_text_c_num ) {;
>
> You could write the whole thing as
>
> if ( ((unsigned int)ct) <= cons_text_c_num ) {;
> but that may be a little too clever for its own good, especially
> since the performance of this code can’t matter.
>
> -- Jim
>
> James Cownie <james.h.cownie at intel.com>
> SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
>
> Tel: +44 117 9071438
>
>
>
>
>
> From: openmp-dev-bounces at cs.uiuc.edu
> [mailto:openmp-dev-bounces at cs.uiuc.edu] On Behalf Of Carlo Bertolli
> Sent: Friday, February 27, 2015 10:57 PM
> To: openmp-dev at dcs-maillist2.engr.illinois.edu
> Subject: [Openmp-dev] Two warnings with clang 3.5 on ppc63le
>
>
>
> Hi
>
> Using clang 3.5 on ppc64le, I obtained the following warnings.
>
> 1.
> /home/compteam/slave0/build_folder_omp/libiomp-src/runtime/src/kmp_error.c:124:23:
> warning: comparison of constant 16 with expression of type 'enum
> cons_type' is always true
> [-Wtautological-constant-out-of-range-compare]
> if ( 0 < ct && ct <= cons_text_c_num ) {;
> ~~ ^ ~~~~~~~~~~~~~~~
>
> It looks like that there are no machines where const char * is more
> that 64-bit. The second check can be removed safely - maybe the
> author of this intended something different?
>
> 2.
> /home/compteam/slave0/build_folder_omp/libiomp-src/runtime/src/kmp_runtime.c:270:58:
> warning: multiple unsequenced modifications to 'gtid'
> [-Wunsequenced]
> if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid =
> __kmp_gtid_from_thread( th )))
> ^
> /home/compteam/slave0/build_folder_omp/libiomp-src/runtime/src/kmp.h:958:25:
> note: expanded from macro 'KMP_UBER_GTID'
> (__kmp_threads[(gtid)] == __kmp_root[(gtid)]->r.r_uber_thread)\
> ^
>
> gtid here is modified multiple times. We can just move the assignment
> out of the if and have only gtid as parameter of the macro.
>
>
> Below a patch that fixes these problems. Please let me know your
> thoughts on this.
>
> Thanks
>
> -- Carlo
>
>
>
>
> diff --git a/runtime/src/kmp_error.c b/runtime/src/kmp_error.c
> index b76c109..4274365 100644
> --- a/runtime/src/kmp_error.c
> +++ b/runtime/src/kmp_error.c
> @@ -121,7 +121,7 @@ __kmp_pragma(
> kmp_str_buf_t buffer;
> kmp_msg_t prgm;
> __kmp_str_buf_init( & buffer );
> - if ( 0 < ct && ct <= cons_text_c_num ) {;
> + if ( 0 < ct ) {
> cons = cons_text_c[ ct ];
> } else {
> KMP_DEBUG_ASSERT( 0 );
> diff --git a/runtime/src/kmp_runtime.c b/runtime/src/kmp_runtime.c
> index 55b58ce..0a08993 100644
> --- a/runtime/src/kmp_runtime.c
> +++ b/runtime/src/kmp_runtime.c
> @@ -267,7 +267,8 @@ __kmp_check_stack_overlap( kmp_info_t *th )
> }
>
> /* No point in checking ubermaster threads since they use refinement
> and cannot overlap */
> - if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid =
> __kmp_gtid_from_thread( th )))
> + gtid = __kmp_gtid_from_thread( th );
> + if ( __kmp_env_checks == TRUE && !KMP_UBER_GTID(gtid))
> {
> KA_TRACE(10,("__kmp_check_stack_overlap: performing extensive
> checking\n"));
> if ( stack_beg == NULL ) {
>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> _______________________________________________
> Openmp-dev mailing list
> Openmp-dev at dcs-maillist2.engr.illinois.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/openmp-dev
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/openmp-dev/attachments/20150302/21cceac0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-dev/attachments/20150302/21cceac0/attachment.gif>
More information about the Openmp-dev
mailing list