support files with 2^31 or more PRNs #25
Conversation
libdieharder/rng_file_input.c
Outdated
| } | ||
| if(strncmp(splitbuf[0],"count",5) == 0){ | ||
| state->flen = atoi(splitbuf[1]); | ||
| state->flen = atoll(splitbuf[1]); |
There was a problem hiding this comment.
This is the main change. count might be larger than INT_MAX, thus atoll is preferred.
| state->rptr++; | ||
| state->rtot++; | ||
| if(verbose){ | ||
| fprintf(stdout,"# file_input() %lu: %lu/%lu -> %u\n", state->rtot, state->rptr,state->flen,(uint)iret); |
There was a problem hiding this comment.
Note that the original off_t is an alias for the signed type long int. Furthermore, one can never know what off_t actually is. Could be any signed 64-bit-integer, e.g., long int or long long int on Linux on x64, which are distinct types with distinct format strings.
| FILE *fp; | ||
| off_t flen; | ||
| off_t rptr; | ||
| off_t rtot; |
There was a problem hiding this comment.
There is no good reason for using off_t. No POSIX api is used, which requires off_t.
|
While we're at it, should we modernise this use of |
and improve error handling in parameter parsing
|
@eddelbuettel I updated the PR incorporating your suggestion. |
eddelbuettel
left a comment
eddelbuettel
left a comment
There was a problem hiding this comment.
This looks like a nice enhancement!
|
@mbroz Before I merge it, do you mind having a look at it too? |
|
Well, I am afraid this will not be so simple. First, this is the ABI/API libdieharder break, which modifies exported prototypes in libdieharder.h (I guess the Debian build will scream for this). Anyway, this will probably not be a simple place where a 32-bit count is used... |
|
If ABI/API stability is a concern, I could revise the change to use I was not aware that the behaviour of |
|
I think most of this patch is actually not needed. dieharder.h alteady contains line Perhaps we can add AC_SYS_LARGEFILE macro to configure.ac that will switch off_t to 64bit in config.h globally. But even without that, I see sizeof(off_t) 8-bytes (64bits) even on 32bit platforms. So why we need to redefine off_t to different type? Perhaps only CLI parsing fix is needed? |
|
And I generally like both to simplify, and to keep changes small and limited. @rabauke raised a valid point: we can / could / should support larger files. I still see as a bit of edge case. Maybe we can try to address that more narrowly? |
|
I think that redefinition of _FILE_OFFSET_BITS already switches everything to large file support. Compilation on 32bit produces some warnings (some of them will be off_t in printf for sure), but these should be easily fixed. I am not sure anyone uses dieharder on 32bit systems, but with large file support it should work identically to 64bit systems. So the only problem here is to properly handle 64bit off_t in terminal input/output. Or do I miss anything here? |
| } | ||
| } | ||
| if(strncmp(splitbuf[0],"numbit",6) == 0){ | ||
| filenumbits = atoi(splitbuf[1]); |
There was a problem hiding this comment.
one more comment - filenumbits is int, so atoi is enough here. Just it should check for 0 as error (0 does not make sense anyway here)
3 participants
Given the typical storage capacities of recent hardware, dieharder should support reading PRNs from files with more than 2^31 numbers.