r3.in.v5d: Prevent integer overflow by changing literal constant type on Cray

[ymdatta]

When the code is being compiled for CRAY HPC machines, a macro and a function to convert IEEE single precision floating point number to CRAY number are defined.

To adjust the base, '16258' constant is being used, which according to C rules (C99, section 6.4.4.1, subsection semantics) fits into an integer. Right shifting that integer, which is of 32 bits, by 48 results in integer overflow. Avoid this by defining the literal constant with the long data type.

[wenzeslaus]

Min size of long int is 32 so that still can overflow. I get -Wshift-count-overflow. I think LL is needed.

[ymdatta]

@wenzeslaus : Thank you for pointing that issue! I spent good amount of time thinking about it earlier.

If for example long was 32 bits on the machine where this is being compiled, then we would be running into additional issues while performing other operations in the same function.

Ex: (*f & 0x7f80000000000000) >> 7), *f which would be of long would have to be converted to long long to ensure both sides of binary operator ( & ) have same type. This would mean extending the MSB to 32 more bits in *f which would totally mess up with our calculations as our intention for example in the operation here is calculate and store bits 24 to 31 in *f. Shifting again would cause issues.

So, I assumed it would be safe as long. I think this is intended for CRAY systems only which have 64 bits for long.

Using long long also works because at the end, we would only be using first half and truncating the remaining part. I changed my code to use LL for the constant literal just to be safe.

(I was also a bit worried if CRAY supports long long, but gcc as a standard would take care of this I believe when source is compiled on the machine using gcc, I am a bit unclear about this to be honest)

I am also curious about how you got to this -Wshift-count-overflow, I didn't get that as part of my workflow.

[wenzeslaus]

Looking into this more, I don't find clear info on when _CRAY would be defined, but it seems to me that it is mostly case for old Cray machines. New Cray compilers support new C standards. This _CRAY-specific place in r3.in.v5d is the only place in the code base, so let's move on.

[wenzeslaus]

I am also curious about how you got to this -Wshift-count-overflow, I didn't get that as part of my workflow.

One of the online C/C++ compilers :-) I checked just the expression, not the whole file or source code.

[nilason]

The CRAY additions were made in mid-90s, some time ago. Not likely that code is relevant anymore. So long or long long probably doesn't matter much. (However the function parameters are longs and thus the variables involved in the bit shift, so too I believe should be the literals in this fix.)

[wenzeslaus]

Sorry @ymdatta, can you please change this to be consistent with the rest of the code by using L not LL as I previosly suggested. We will let -Wshift-count-overflow bite us later and in the meantime, perhaps we can remove or otherwise revise this code.

[nilason]

We will let -Wshift-count-overflow bite us later and in the meantime, perhaps we can remove or otherwise revise this code.

We may also consider remove the "CRAY" code, unless someone is actually using GRASS GIS with present CRAY (and would be able to assist...).

[nilason]

The code was probably originated from https://sourceforge.net/projects/vis5d/

(see mirror at https://github.com/drafnel/Vis5d/blob/master/src/binio.c)

[neteler]

Seems I have added it to GRASS GIS 25 years ago (on May 2, 2000):

OSGeo/grass-legacy@09c5679#diff-45d5ff840b803deffa0d4cbc279dc064524276c0350c4630036d7a2e13a75737

but don't see any related discussion in the grass-dev archive...

[ymdatta]

Modified the literal back to long.

Even the magic number in the code 16258 is something I'm unsure of. We are doing exponent conversion from IEEE to CRAY in accordance with the based form. I spent a lot of time learning about these formats and I find that it should actually be 16256 rather than 16258.

Let's say, if exponent in the IEEE format is E, the actual value it's representing is E - 127. Similarly, if exponent in the CRAY format is T, the actual value it's representing is T - 16383. Now, if we are converting from IEEE format to CRAY format, we would want these 'actual values' to be the same, so E - 127 = T - 16383 i.e E + 16256 = T. Value stored in CRAY exponent is equal to value stored in IEEE exponent + 16256. I am not able to understand why it's 16258 instead. I really want to know if I am wrong somewhere.

But, it took me some time going through CRAY manuals to get here. If we are not using this anywhere, I would also suggest removing it to keep the code simple.

[wenzeslaus]

@ymdatta Sorry, realized only now this was still pending. Can you please remove the Cray code in a new PR?

[ymdatta]

@wenzeslaus : No problem. I'll open a PR removing Cray specific code.