openshmem-org / specification

OpenSHMEM Application Programming Interface
http://www.openshmem.org
49 stars 32 forks source link

Resolve compiler warning in 2D team split example #449

Closed davidozog closed 3 years ago

davidozog commented 3 years ago

The Clang compiler (at least) warns about not initializing the x, y, and z dimensional variables in the shmem_team_split_2D.c example code.

This is pretty low priority because the find_xyz_dims function should successfully set these variables (unless shmem_n_pes or cbrt(npes) return something unexpected) but we should fix it anyway. An error check on these variables after find_xyz_dims is an alternative approach.

davidozog commented 3 years ago

@jdinan - is it ok to merge this now?

jdinan commented 3 years ago

Is it correct that because npes >= 1 the minimum value of these dimensions is also 1? Another way to handle this would be to set *x = *y = *z = 1 in find_xyz_dims or possibly to assert(npes >= 1) to silence the warning in that function. I'm not sure which of these I prefer. Can you help me decide?

davidozog commented 3 years ago

Yeah I think that's correct. I'd probably prefer to assert(npes >=1) if it indeed silences the warning...

But I have my doubts it will - so I just checked, and unfortunately I can't get the warning to show up at all anymore... I tried too many compilers - Apple clang v12.0.0, Ubuntu clang v10.0.0-4ubuntu1, and Ubuntu gcc v9.3.0, v5.4.0, and 4.8.4, passing all the extra warning flags.

This came about from https://github.com/Sandia-OpenSHMEM/SOS/issues/972. So it seems unique to Travis (with -Wmaybe-uninitialized) but I have an environment almost identical to Travis xenial that won't print the warning.

Do you recall seeing this warning in the wild by change @jdinan?

Anyway, let me run it through Travis and I'll get back to you. I have a feeling this might be of no practical importance.

jdinan commented 3 years ago

I have not personally observed this warning, but I can see how a compiler might emit this warning given how the variables are initialized.

davidozog commented 3 years ago

@jdinan - Yeah, interestingly the warning still shows up in Travis CI. An assert on npes >=1 alone still prints the warning... and initializing in find_xyz_dims eliminates the warning.

I think it's best to do set *x = *y = *z = 1 in find_xyz_dims like you suggest (users of a helper function shouldn't have to initialize output variables). I don't think the assert is necessarily needed - I'd bet most of the spec example programs would break if npes <= 0 😉. Assuming you agree, I'll make that change here. Would this be a document edit?

jdinan commented 3 years ago

@davidozog That sounds good. Yes, happy to handle this as a doc edit.

davidozog commented 3 years ago

There's a funny problem in the CI build during sudo apt-get update:

...
Reading state information...
E: Unable to locate package texlive-generic-recommended
Error: Process completed with exit code 100.

is that new?