Undefined behaviour in regression suite

[Joshua Landau]

The regression suite, files

• systemc-regressions-2.3.1a/tests/systemc/datatypes/fx/constructors/array.cpp
• systemc-regressions-2.3.1a/tests/systemc/datatypes/fx/fast_constructors/array.cpp
• systemc-regressions-2.3.1a/tests/systemc/datatypes/misc/test02/test02.cpp

invoke undefined behaviour by converting out-of-range doubles to unsigned integers directly. This is undefined in the C++ standard

Quote

When a finite value of real floating type is converted to an integer type other than _Bool, the fractional part is discarded (i.e., the value is truncated toward zero). If the value of the integral part cannot be represented by the integer type, the behavior is undefined.

See http://stackoverflow.com/a/4752947/1763356 for further information.

This happens multiple most prominently in the code

sc_fxval *b = new sc_fxval[4];
b[0] = (ushort)-1;
for (i = 1; i < 4; ++i)
  b[i] = (ushort)(b[i-1] * i * -1);

where negative doubles are converted to unsigned shorts, because sc_fxval only has conversions to double. It is unclear whether the test is correct and sc_fxval wrong, or vice-versa. If the later, one can avoid undefined behaviour by indirecting through a sufficiently large signed type.

sc_fxval *b = new sc_fxval[4];
b[0] = (ushort)-1;
for (i = 1; i < 4; ++i)
  b[i] = (ushort)(int64)(b[i-1] * i * -1);

I was caught by this issue when testing a build of SystemC for AArch64, as part of a port I am making. As an aside, how (and where) should I attempt to upstream my port when it is ready?

 

Edited January 13, 2017 by Joshua Landau
Typo

[maehne]

Dear Joshua,

Thank you for your bug report! I have forwarded the issue to the internal issue tracker of the SystemC Language Working Group so that it can be examined and fixed in a future version of the regression test suite.

Regarding your port to AArch64, you can post about the necessary modifications in this forum. There is also an upload area accessible via a link on the SystemC Community page of the Accellera website. For possible integration, your modifications have to be licensed under the Apache license.

Best regards,

Torsten Maehne

[Philipp A Hartmann]

Hi Joshua,

thanks for the report also from my side.

I think, a better fix for the regression tests in question would be to use one of the explicit conversion functions in sc_fxval(_fast), see e.g. IEEE 1666-2011, 7.10.12.2:

b[i] = (ushort)(b[i-1].to_ushort() * i * -1);

The other question would be: Is the undefined behavior actually triggered?  The quoted code looks like the sc_fxval values should indeed be positive as they have been assigned from a positive (ushort) value.  The resulting double value then definitely holds a number that fits back into the unsigned short.

Can you elaborate?

Greetings from Duisburg,
  Philipp

[Joshua Landau]

Thank you both.

6 hours ago, Philipp A Hartmann said:

I think, a better fix for the regression tests in question would be to use one of the explicit conversion functions in sc_fxval(_fast), see e.g. IEEE 1666-2011, 7.10.12.2:

The explicit functions are implemented much the same way;

static_cast<unsigned short>( m_rep->to_double() )

However, I expect you are right that doing the conversion before, rather than after, the multiplication by -1 is a better implementation than I gave. To elaborate, the code was invoking undefined behaviour before because b[i-1] was implicitly converting to double, which resulted in

b[i-1] * i * -1

being a negative double, which was only then cast to ushort. An early cast would mean the multiplication results in an integral (likely of type int), which will then get truncated safely.

On the other hand, this can cause UB:

(uint)(sc_fix)(uint)-1

sc_fix wraps the (uint)-1 result back around to negative. Thus, this won't help if b[i-1] is ever expected to go over 32768, which it is.

[Philipp A Hartmann]

Hi Joshua,

On 1/16/2017 at 0:07 PM, Joshua Landau said:

On the other hand, this can cause UB:

(uint)(sc_fix)(uint)-1

sc_fix wraps the (uint)-1 result back around to negative. Thus, this won't help if b[i-1] is ever expected to go over 32768, which it is.

This could then indeed be a bug.
I would expect that

(sc_fix)(uint)-1

is a positive value?  You say, that this is not the case?

Thanks,
  Philipp

[Joshua Landau]

I don't have the test files on hand to double-check, but IIRC I believe it gets turned negative during the overflow step (called by cast, in turn called by sc_fxnum::sc_fxnum).

Bear in mind that I'm new to SystemC, so I don't have a clear idea of what behaviours are expected or vice-versa.