{"action":"created","issue":{"url":"https://api.github.com/repos/NCAR/ParallelIO/issues/79","repository_url":"https://api.github.com/repos/NCAR/ParallelIO","labels_url":"https://api.github.com/repos/NCAR/ParallelIO/issues/79/labels{/name}","comments_url":"https://api.github.com/repos/NCAR/ParallelIO/issues/79/comments","events_url":"https://api.github.com/repos/NCAR/ParallelIO/issues/79/events","html_url":"https://github.com/NCAR/ParallelIO/issues/79","id":161245565,"number":79,"title":"not correct to cast MPI_Offset pointers to size_t pointers","user":{"login":"edhartnett","id":4533212,"avatar_url":"https://avatars.githubusercontent.com/u/4533212?v=3","gravatar_id":"","url":"https://api.github.com/users/edhartnett","html_url":"https://github.com/edhartnett","followers_url":"https://api.github.com/users/edhartnett/followers","following_url":"https://api.github.com/users/edhartnett/following{/other_user}","gists_url":"https://api.github.com/users/edhartnett/gists{/gist_id}","starred_url":"https://api.github.com/users/edhartnett/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/edhartnett/subscriptions","organizations_url":"https://api.github.com/users/edhartnett/orgs","repos_url":"https://api.github.com/users/edhartnett/repos","events_url":"https://api.github.com/users/edhartnett/events{/privacy}","received_events_url":"https://api.github.com/users/edhartnett/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2016-06-20T17:07:22Z","updated_at":"2016-08-01T15:40:49Z","closed_at":null,"body":"NetCDF uses size_t for sizes of things, like the size of data arrays,\r\n\r\nPIO uses PIO_Offset for this purpose. PIO_Offset is defined in pio.h to be MPI_OFFSET, or MPI_LONG_LONG if MPI_OFFSET is not defined.\r\n\r\nAs far as I can tell from the MPICH mpi.h header file, it is using long long as MPI_OFFSET.\r\n\r\nThe size_t type is an unsigned long long on every platform I'm familiar with.\r\n\r\nIf ever the size of MPI_OFFSET and size_t are different, then the PIO code will fail quite dramatically. But that is unlikely. It's hard to imagine an MPI platform that is not going to use a 64-bit int of some kind. Similarly size_t is a 64-bit int in every modern C compiler.\r\n\r\nHowever, the real problem is that size_t is unsigned, and MPI_OFFSET is signed. Can it be that someone could be tryping to set things to sizes larger than the maximum signed 64-bit integer? (2^^63 − 1 or 9,223,372,036,854,775,807). I don't know, that's a pretty big number.\r\n\r\nAn additional source of confusion is that both PIO_OFFSET and PIO_Offset are defined, as are MPI_OFFSET and MPI_Offset. \r\n\r\nIt is PIO_Offset that is actually used in the code, and that is defined to be MPI_Offset, which is defined in pio_internal.h to be a long long. So MPI_OFFSET is not actually used.\r\n\r\nSome possible approaches:\r\n\r\n* Would be nice to remove PIO_OFFSET and MPI_Offset, leaving MPI_OFFSET (an MPI defined macro, in most cases) and PIO_Offset, which is used all over the code. We should probably set PIO_Offset to MPI_OFFSET, since that is what the user will expect.\r\n\r\n* Would be nice to confirm that there is even a possibility of an issue. Some things (such as the ndims) cannot be larger than other number NC_MAX_DIM, so by carefully looking at the netCDF api perhaps we can find that there is no possibility of even having numbers larger than 2^15-1.\r\n\r\n* Right now we are just overriding all the warnings by casting MPI_OFFSET pointers to size_t pointers. This is a bit sleazy - we should find a better way.\r\n\r\n* We could create (in all functions that currently cast) size_t arrays, copy values from MPI_OFFSET arrays, and then we don't have to cast. Probably this is the best solution, though somewhat laborious.\r\n\r\n* We could change PIO_OFFSET to size_t, but that would introduce these warnings in all existing user codes. :-(\r\n\r\n* We could check the size of MPI_OFFSET vs. size_t in the CMake step. If they are different, then we notify the user and fail to build. That way we can rely on safely casting between these types. (But if anyone ever does build a system where MPI_OFFSET and size_t are different sizes, PIO is hosed and needs major editing to work there.)\r\n\r\nAny suggestions welcome. I wanted to capture this in an issue because I will probably forget all about it when I go on vacation next week. ;-)"},"comment":{"url":"https://api.github.com/repos/NCAR/ParallelIO/issues/comments/236618538","html_url":"https://github.com/NCAR/ParallelIO/issues/79#issuecomment-236618538","issue_url":"https://api.github.com/repos/NCAR/ParallelIO/issues/79","id":236618538,"user":{"login":"edhartnett","id":4533212,"avatar_url":"https://avatars.githubusercontent.com/u/4533212?v=3","gravatar_id":"","url":"https://api.github.com/users/edhartnett","html_url":"https://github.com/edhartnett","followers_url":"https://api.github.com/users/edhartnett/followers","following_url":"https://api.github.com/users/edhartnett/following{/other_user}","gists_url":"https://api.github.com/users/edhartnett/gists{/gist_id}","starred_url":"https://api.github.com/users/edhartnett/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/edhartnett/subscriptions","organizations_url":"https://api.github.com/users/edhartnett/orgs","repos_url":"https://api.github.com/users/edhartnett/repos","events_url":"https://api.github.com/users/edhartnett/events{/privacy}","received_events_url":"https://api.github.com/users/edhartnett/received_events","type":"User","site_admin":false},"created_at":"2016-08-01T15:40:49Z","updated_at":"2016-08-01T15:40:49Z","body":"I need to check carefully to see if it is ever even possible for this to cause a problem, before any action is taken. So I will do that. It may be just fine as it is, in which case I will document my results and move on. I just didn't want to forget this issue. ;-)\r\n\r\nThe parallel-netcdf library does not call the netCDF library directly, so does not face the same problem. They use MPI_Offset (instead of size_t) in their prototypes. I see at least one cases where they are casting a size_t to an MPI_Offset, but using a macro to check that it is not too large.\r\n\r\nIn terms of performance, it will only be a factor in the data reads and writes. Metadata operations all take place once, whereas data operations take place over and over. As far as I can see there is no way to have a valid start, count, or stride which is longer than NC_MAX_DIM, so we can just check at build time that this is OK and we're good. That leaves the metadata functions to check for potential issues.\r\n\r\n"}}
{"actor":{"display_login":"edhartnett"}}