#223: Make declaration of CCTK_ARGUMENTS safer -------------------------+-------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: Keywords: | -------------------------+-------------------------------------------------- I suggest to change the declaration of CCTK_ARGUMENTS in C from
cGH * cctkGH
to
cGH const * CCTK_RESTRICT const cctkGH
which should lead to safer code and may even enable some optimisations.
#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by bmundim):
I was wondering what the state of this proposal is. It seems a good idea to me...
#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by eschnett):
After this change, it is not possible any more to modify cctkGH entries directly. At least thorns do this -- CartGrid3D and Time modify the cctk_origin_space, cctk_delta_space, and cctk_delta_time entries. These thorns will need to be modified to cast away the const declaration.
#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by eschnett):
* status: new => review
#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by knarf):
How save it is to cast away a const declaration? Couldn't this not only cause a compiler warning, but also wrong execution because the compiler might have assumed variables to be const when in fact they were changed using a cast?
#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by rhaas):
In C casting away const is a valid operation. Unless there is also a restrict keyword present the compiler cannot assume that an object '''pointed to''' by a const pointer cannot change anyway since C allows aliasing, ie this is valid code that does no invoke undefined behaviour:
{{{ void mystrcpy(char *s, const char *t) { while(*t) *s++ = *t++; } int main(void) { char blah[] = "foo"; mystrcpy(blah, blah); printf("%s\n", blah); return 0; } }}}
Changing a const object directly is invalid though (compilers are free to allow it though, it seems to be "should" behaviour at least in C89).
C++ might differ (I remember having had a hard time getting rid of a const once but then I don't know much of the darker corners of C++). Even with a restrict argument casting away const should be fine since the compiler can trace data dependece between the two pointers (though I am not sure what the standard says about creating a secondary pointer from a restricted pointer).
Casting away const is not nice of course :-)
#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by eschnett):
Casting away const in C++ is done one a few occasions; const_cast<>() is a good way to do so.
In Cactus, we need to cast away const in thorn CartGrid3D (when setting cctk_delta_space) and in thorn time (when setting cctk_delta_time). Both are inelegant, and should be replaced by a flesh API instead. However, what we currently have works. And since it only involves C, it is safe, as Roland explains.
#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
This change is a very good idea and should be implemented.
Process:
For the benefit of anyone reading cGH-const.diff, I think the first, second and fourth hunks can be ignored, as they don't seem to be related to this change. I'm not sure what the first hunk is doing, but it is not just a formatting change, so should probably not be committed without being remarked on or put into a separate patch.
Content:
I'm a bit confused about the relation between the summary and comments and the patches. The summary says that cctkGH is made const, whereas it looks to me like the patch makes the local variables which are computed from cctkGH const. My naive interpretation would be that you could still modify variables if you accessed them via cctkGH->... and this seems to be what the CartGrid3D patch is doing. When you say in comment:2 that CartGrid3D and Time need to be modified, are you talking about a further modification to CartGrid3D beyond the current patch, for example one that casts away constness?
The approach taken in the patch looks to me like a good one, and should help to avoid nasty user bugs. Is there a similar patch for the Time thorn, as the summary indicates that this will be needed before these can be committed. Also, for such a pervasive change, running the ET test suite on a couple of architectures might shake out any problems before users running on trunk get bitten by them. Do you want to just commit this and see what falls out, or test it thoroughly before it is committed?
#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: Cactus | Version: Resolution: fixed | Keywords: --------------------------+------------------------------------------------- Changes (by eschnett):
* status: review => closed * resolution: => fixed
Comment:
The first hunk was only present because the patches were based on an older version of Cactus. This hunk has been committed independently since then.
Yes, it is still possible to modify the content of (the structure pointed to by) cctkGH.
No, thorn time does not need to be modified. Since cctk_delta_space is an array, its corresponding local variable aliases the one in cctkGH, so it used to be possible to modify the cctkGH by modifying this alias, but since this alias is now declared const, this is not possible any more. However, cctk_delta_time is a scalar, and since C doesn't support references to scalars, the corresponding local variable is a copy, and therefore one cannot modify cctkGH by modifying the local variable, and thorn Time therefore already modifies the content of cctkGH directly.
I have been building and running with these patches for ages, they work fine on all architectures, I am happy to finally commit them.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit