Hi
Using clang 3.5 on ppc64le, I obtained the following warnings.
-
/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?
-
/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 ) {
-
/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, 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.
From: "James H Cownie" <james.h.cownie@intel.com>
To: "Carlo Bertolli" <cbertol@us.ibm.com>, openmp-dev@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
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@intel.com>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: +44 117 9071438
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
“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
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…
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(
- 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 ) {
"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
Hi Carlo,
The patch looks good to me.
I have committed it, svn rev. 231078.
Thanks,
Andrey
