accellera-official / systemc

SystemC Reference Implementation
https://systemc.org/overview/systemc/
Apache License 2.0
475 stars 152 forks source link

Undefined Behaviour in sc_int constructor #61

Closed mpardalos closed 9 months ago

mpardalos commented 10 months ago

The following code gives an error when compiled in clang with -fsanitize=undefined:

#include <systemc>

int sc_main(int, char**) {
    sc_dt::sc_int<32> x = -1;
    std::cout << x << std::endl;
    return 0;
}

The output:

        SystemC 2.3.3-Accellera --- May 18 2021 00:00:00
        Copyright (c) 1996-2018 by all Contributors,
        ALL RIGHTS RESERVED
/usr/include/systemc/sysc/datatypes/int/sc_int_base.h:574:22: runtime error: left shift of negative value -1
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/include/systemc/sysc/datatypes/int/sc_int_base.h:574:22 in 
-1

The relevant code:

https://github.com/accellera-official/systemc/blob/39740075f47fedbce242808d357fcfb6d3d33957/src/sysc/datatypes/int/sc_int_base.h#L569-L575

The result is correct in this example, however there could be cases where, depending on optimisation level, the compiler could do something unexpected with this code. Perhaps there should be a check for negative values before doing the shift here?

maehne commented 10 months ago

Thanks for reporting this issue!

@AndrewGoodrich: Could you maybe have a look whether this has already been fixed by your recent changes?

AndrewGoodrich commented 10 months ago

I do not get a warning or error with the recent changes using:

include

int sc_main(int, char**) { sc_dt::sc_int<32> x = -1; std::cout << x << std::endl; return 0; }

It produces the expected output:

    SystemC 3.0.0_pub_rev_20231024-Accellera --- Nov  7 2023 10:51:15
    Copyright (c) 1996-2023 by all Contributors,
    ALL RIGHTS RESERVED

-1

On Nov 14, 2023, at 3:04 AM, Torsten Maehne @.***> wrote:

Thanks for reporting this issue!

@AndrewGoodrich https://github.com/AndrewGoodrich: Could you maybe have a look whether this has already been fixed by your recent changes?

— Reply to this email directly, view it on GitHub https://github.com/accellera-official/systemc/issues/61#issuecomment-1809711860, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGQR2OLXTFDJA3ZGHKVBSTYEMQ2XAVCNFSM6AAAAAA7JO3AWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBZG4YTCOBWGA. You are receiving this because you were mentioned.

johnwickerson commented 10 months ago

Hi @AndrewGoodrich, did you enable -fsanitize=undefined when compiling?

AndrewGoodrich commented 10 months ago

I did with the test shown in the PR, I did not compile a library with it set.

Sent from my iPad

On Nov 15, 2023, at 8:41 AM, John Wickerson @.***> wrote:

 I do not get a warning or error with the recent changes using: #include int sc_main(int, char**) { sc_dt::sc_int<32> x = -1; std::cout << x << std::endl; return 0; } It produces the expected output: SystemC 3.0.0_pub_rev_20231024-Accellera --- Nov 7 2023 10:51:15 Copyright (c) 1996-2023 by all Contributors, ALL RIGHTS RESERVED -1 … On Nov 14, 2023, at 3:04 AM, Torsten Maehne @.***> wrote: Thanks for reporting this issue! @AndrewGoodrich https://github.com/AndrewGoodrich: Could you maybe have a look whether this has already been fixed by your recent changes? — Reply to this email directly, view it on GitHub <#61 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGQR2OLXTFDJA3ZGHKVBSTYEMQ2XAVCNFSM6AAAAAA7JO3AWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBZG4YTCOBWGA. You are receiving this because you were mentioned.

Hi @AndrewGoodrich, did you enable -fsanitize=undefined when compiling?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

mpardalos commented 10 months ago

Just to double-check we are all on the same page, this is how I got this error:

~/Documents/PhD/systemc-test
❯ cat ub.cpp
#include <systemc>

int sc_main(int, char**) {
    sc_dt::sc_int<32> x = -1;
    std::cout << x << std::endl;
    return 0;
}
~/Documents/PhD/systemc-test
❯ clang++ -fsanitize=undefined -I/usr/include/systemc -std=c++11 -lsystemc ub.cpp
~/Documents/PhD/systemc-test
❯ ./a.out

        SystemC 2.3.3-Accellera --- May 18 2021 00:00:00
        Copyright (c) 1996-2018 by all Contributors,
        ALL RIGHTS RESERVED
/usr/include/systemc/sysc/datatypes/int/sc_int_base.h:574:22: runtime error: left shift of negative value -1
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/include/systemc/sysc/datatypes/int/sc_int_base.h:574:22 in
-1
pah commented 10 months ago

This issue was resolved in osci-wg/systemc#364.

AndrewGoodrich commented 10 months ago

The test I used is:

include

int sc_main(int, char**) { sc_dt::sc_int<32> x = -1; std::cout << x << std::endl; return 0; }

The same as you used. It does not cause a problem.

Note the issue with the 2.3.x library, as pointed out by PAH is sign extension by using shifts of a signed value, m_val, which is negative i the test case:

  m_val = ( m_val << m_ulen >> m_ulen );

The 3.0 library with the new support for large integer types, that I test agaist uses:

        if ( m_val & (1ull << (m_len-1)) ) {
            m_val |= (~UINT_ZERO << (m_len-1));
        }
        else {
            m_val &= ( ~UINT_ZERO >> m_ulen );
        }

So there is no shift of a negative number.

On Nov 15, 2023, at 2:36 PM, Michalis Pardalos @.***> wrote:

Just to double-check we are all on the same page, this is how I got this error:

~/Documents/PhD/systemc-test ❯ cat ub.cpp

include

int sc_main(int, char**) { sc_dt::sc_int<32> x = -1; std::cout << x << std::endl; return 0; } ~/Documents/PhD/systemc-test ❯ clang++ -fsanitize=undefined -I/usr/include/systemc -std=c++11 -lsystemc ub.cpp ~/Documents/PhD/systemc-test ❯ ./a.out

    SystemC 2.3.3-Accellera --- May 18 2021 00:00:00
    Copyright (c) 1996-2018 by all Contributors,
    ALL RIGHTS RESERVED

/usr/include/systemc/sysc/datatypes/int/sc_int_base.h:574:22: runtime error: left shift of negative value -1 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/include/systemc/sysc/datatypes/int/sc_int_base.h:574:22 in -1 — Reply to this email directly, view it on GitHub https://github.com/accellera-official/systemc/issues/61#issuecomment-1813145564, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGQR2LXF3E4XOAGZT6JECTYEUKSZAVCNFSM6AAAAAA7JO3AWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJTGE2DKNJWGQ. You are receiving this because you were mentioned.

lmailletcontoz commented 9 months ago

Fixed in SystemC 3.0.0 release. Please reopen in case of regression.