Re: Unexpected change in behaviour of ai/ao device support

["Johnson, Andrew N. via Tech-talk" <tech-talk@aps.anl.gov>]

Hi Michael,

If you have problems following email quoting, the tech-talk archive seems to do a reasonable job with displaying most messages.

On 6/12/19 1:32 AM, michael.abbott@diamond.ac.uk wrote:

In you anxiety to avoid the “undefined” wrapping that occurs with integer arithmetic (technically, undefined for signed, defined as modulo word size for unsigned), you changed the semantics drastically.  This doesn’t feel like a good trade off to me.

Optimizing compilers nowadays are becoming much better at generating code that doesn't do what we think it should because our code happens to allow something that isn't strictly legal (but worked fine with a compiler from a few years ago). I'd rather be proactive in removing bad code while I'm actually working on it than have some compiler upgrade find it for us at a bad time.

I admit I did change the semantics, but not for the device support routines that I have access to because they all used positive or zero values for RVAL. The ai and ao records were designed to interface to individual ADCs and DACs, which almost invariably give/take either signed or offset zero integer values of various bit-widths. RVAL was designed to support the offset zero types, for which a zero analog value corresponds to a positive digital value (which is what you put into RVAL). That functionality still works perfectly with the new convert() code, and can now support the full 32-bit range that Dirk Zimoch was asking for at the time.
 

Ignoring your boundary checking and the rounding, and with extra casts inserted to make the types involved clearer, the difference between the two versions of the conversion code boils down to this:

 

old:

                rval = (int32_t) (double) value - (int32_t) roff;

So rval = <int32_t> - <int32_t> which was fine as long as the subtraction didn't result in an overflow, since that would be UB.
 

Now let’s look at a value of 0 being adjusted with an roff of -512 (for whatever reason, this is what the Hy8402 driver does):

 

old:

                rval = 512;

Now let's look at the value 2147483392=0x7fffff00 instead; subtract -512=-0x200 from that and you get an int32_t overflow and hence UB.

new:

                rval = - (uint32_t) -512 = -4294966784;

That's not what gets put into RVAL, which is an int32_t so can't hold that anyway. The RVAL I actually get from the new code is -2147483648=0x80000000 because of the bounds checking, which was added to prevent signed overflows from taking place.

You weren't really supposed to put a negative number into ROFF (the Record Reference describes the field as "an offset to be subtracted from the adjusted value"). The fact that Hytec did that was unfortunately a latent bug in their code, which they aren't now around to fix.

I do wonder if it is time to act on the proposal of 1998 (a trifle before my time in this field) and remove these conversions altogether.  For my own driver support I gave up on letting EPICS do the ai/ao conversion long ago and use the unconverted val field instead.  Much simpler and more predictable!

So you're suggesting we break all the still-working drivers as well?

As you say it's possible for the device support to read the VAL field directly (or better the OVAL field, since you do want to support the ao's OROC functionality don't you?), although it would still have to implement LINR conversions and ASLO/AOFF adjustments itself.

- Andrew

-- 
Complexity comes for free, Simplicity you have to work for.