Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opal: do not declare static inline ... (void) #3829

Conversation

ggouaillardet
Copy link
Contributor

just declare inline subroutines with
static inline ... ()
in order to make some compilers (xlc 12.1) happy

Thanks Paul Hargrove for the report

Refs. #3816

Signed-off-by: Gilles Gouaillardet [email protected]

just declare inline subroutines with
static inline ... ()
in order to make some compilers (xlc 12.1) happy

Thanks Paul Hargrove for the report

Refs. open-mpi#3816

Signed-off-by: Gilles Gouaillardet <[email protected]>
@ggouaillardet ggouaillardet added this to the v3.0.0 milestone Jul 7, 2017
@ggouaillardet ggouaillardet requested a review from jsquyres July 7, 2017 08:25
@jsquyres
Copy link
Member

jsquyres commented Jul 7, 2017

bot:mellanox:retest

@jsquyres
Copy link
Member

jsquyres commented Jul 7, 2017

Hmm. I'm not a fan of this fix.

Per #3816, I see that @PHHargrove confirms that it fixes the issue, but it's still... weird.

Is XLC v12.1 an old version of the compiler? (I ask because I have no idea what the current version of XLC is)
Or perhaps was this behavior a bug that was fixed in 12.1.1 (or 12.2 or whatever)?
@gpaulsen @jjhursey Can you guys comment on this (and/or #3816)?

I ask this because I'm a bit hesitant to take valid C89 code and turn it into pre-ANSI C code just to workaround a potential compiler bug.

If XLC 12.1 is a version we care about, is it possible to have some #if statements that do this workaround just for this compiler/scenario, and leave the (void) in all other cases? Or, if a later (free?) upgrade to the compiler fixes the issue, can we just document that that version of the compiler is broken and needs to be upgraded? (like we've done with many other compilers in the README)

@hjelmn
Copy link
Member

hjelmn commented Jul 7, 2017

Yeah, this is odd. static inline void foo (void) {} is valid C99.

@bosilca
Copy link
Member

bosilca commented Jul 7, 2017

There is a semantical difference between the 2, and using void is the explicit way to state there are no arguments. The old notation, the one without the (void), has been deprecated in C99 (6.11.6 Future language directions, Function declarators).

I went back to the original ticket #3816 and I noticed there is nothing to enforce C99 compliance. Should we use c99 instead of xlc ?

@bwbarrett
Copy link
Member

Based on http://public.dhe.ibm.com/software/server/POWER/Linux/xl-compiler/eval/ppc64le/, it would appear that XLC12 is not the most recent compiler, but perhaps IBM doesn't have a linear version history or something I'm missing. I'd rather not support the compiler suite than take this patch.

@jsquyres
Copy link
Member

jsquyres commented Jul 7, 2017

FWIW: hypothetically, xlc supports C99. At least, according to AC_PROG_CC_99. From the config.log that @PHHargrove provided:

configure:14277: checking for xlc option to accept ISO C99
configure:14426: xlc  -c -q32  conftest.c >&5
configure:14426: $? = 0
configure:14439: result: none needed

I didn't check exactly what that test is doing -- it's assumedly checking for some level of C99edness. But it's clearly not checking for this specific feature (or at least, not checking for this feature in the way that paddress.c is using it). But if the compiler at least nominally supports C99, that gives more credence to the assumption that this is simply a compiler bug. If there's a later XLC 12.something that fixes this issue, then per my above comment, I'd suggest we document that XLC 12.whatever on whatever platform is known to be busted; users need to upgrade yadda yadda yadda.

@bwbarrett bwbarrett removed this from the v3.0.0 milestone Jul 7, 2017
@bwbarrett
Copy link
Member

Removing the 3.0.0 milestone, since we're going with the README approach for the release. I'll let @ggouaillardet decide whether to close the PR.

@jjhursey
Copy link
Member

In PR #4103 we removed support for XL compilers < 13.1. So do we need this PR any more?

@ggouaillardet
Copy link
Contributor Author

since we could not reach a consensus on that PR and the issue only occurs on a no more supported compiler, i will close this PR. anyone is free to re-open it if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants