UCS/CPU: Make headers C89 compliant #9911
Conversation
src/ucs/sys/compiler_def.h
Outdated
| @@ -78,7 +78,7 @@ | |||
| #define UCS_BIT(i) (1ul << (i)) | |||
|
|
|||
| /* Mask of bits 0..i-1 */ | |||
| #define UCS_MASK(_i) (((_i) >= 64) ? ~0 : (UCS_BIT(_i) - 1)) | |||
| #define UCS_MASK(_i) (((_i) >= 64) ? ~0ul : (UCS_BIT(_i) - 1)) | |||
There was a problem hiding this comment.
I think this one is intentional, @ivankochin / @yosefe?
There was a problem hiding this comment.
yes, to avoid compiler warnings
There was a problem hiding this comment.
Not on a tertiary, the compiler is promoting everything to ul anyway (even when it is not complaining about it).
There was a problem hiding this comment.
/__w/1/s/contrib/../src/uct/ib/mlx5/ib_mlx5.c:218:26: error: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Werror,-Wconstant-conversion]
cq->cq_length_mask = UCS_MASK(cq->cq_length_log);
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/1/s/contrib/../src/ucs/sys/compiler_def.h:81:50: note: expanded from macro 'UCS_MASK'
#define UCS_MASK(_i) (((_i) >= 64) ? ~0ul : (UCS_BIT(_i) - 1))
^~~~
1 error generated.
There was a problem hiding this comment.
Interesting, the DPC++ compiler choke on this. Without the ul I get this from gcc
.../include/ucs/sys/compiler_def.h:81:50: warning: operand of '?:' changes signedness from 'int' to 'long unsigned int' due to unsignedness of other operand [-Wsign-compare]
81 | #define UCS_MASK(_i) (((_i) >= 64) ? ~0 : (UCS_BIT(_i) - 1))
| ^~
.../include/ucp/api/ucp.h:511:31: note: in expansion of macro 'UCS_MASK'
511 | UCP_DATATYPE_CLASS_MASK = UCS_MASK(UCP_DATATYPE_SHIFT) /**< Data-type class
There was a problem hiding this comment.
We can keep both compilers happy if we use ~0u. Ugly, but it works, there is no demotion of int and no signedness mismatch.
There was a problem hiding this comment.
/scrap/azure/agent-04/AZP_WORKSPACE/2/s/contrib/../test/gtest/common/mem_buffer.cc:321: Failure
Pattern check failed at 0x289dd10 offset 0: Expected: 0x11111111 Actual: 0x1111111111111111
It's related to UCS_MASK change I guess. We invert unsigned 32bits, then convert to 64bits.
There was a problem hiding this comment.
This is puzzling. The current version of the macro does exactly as before, except in unsigned instead of not specified. I'm confused about how did it work before. Was the compiler propagating the type of the lvalue to the 0 in the macro ?
I guess we'll live with the warnings.
|
To unblock CI, please change commit and pr title to something like |
bosilca
changed the title
bosilca
force-pushed
the
fix/picky_compiler_warnings
branch
2 times, most recently
from
f916d70
to
9e96919
Compare
There was a problem hiding this comment.
maybe worth adding a simple test with c89 flag, that includes the relevant headers, to
Line 348 in f7ee29e
This is not a correctness issue but a user friendliness issue. Including some UCX headers while compiling OMPI with picky compiler options generate a ton of warnings. The warning in compiler_def.h is about the two branches of the conditional returning different integer types (one int and the other unsigned long). The other warnings are about declaration of functions with no arguments. The solution here is either adding void to signal there are no arguments or making the declaration a prototype. Fixes openucx#9910 Signed-off-by: George Bosilca <[email protected]>
bosilca
force-pushed
the
fix/picky_compiler_warnings
branch
from
9e96919
to
8a34b22
Compare
This is not a correctness issue but a user friendliness issue. Including some UCX headers while compiling OMPI with picky compiler options generate a ton of warnings.
The warning in compiler_def.h is about the two branches of the conditional returning different integer types (one int and the other unsigned long).
The other warnings are about declaration of functions with no arguments. The solution here is either adding void to signal there are no arguments or making the declaration a prototype.
Fixes #9910