Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Global state in SystemC #8

Open
nmeum opened this issue Sep 22, 2020 · 18 comments
Open

Global state in SystemC #8

nmeum opened this issue Sep 22, 2020 · 18 comments

Comments

@nmeum
Copy link

nmeum commented Sep 22, 2020

I am currently working on a specialized SystemC project where I want to restart the SystemC simulation process multiple times. I have a working proof of concept which does this by providing a custom main function in which I reset the global SystemC simulation context (sc_core::sc_curr_simcontext) and call sc_core::sc_elab_and_sim in a loop. As an example, consider the following code:

int main(int argc, char **argv) {
        // …

	for (i = 0; !exit_simulation; i++) {
		printf("\n##\n# %zuth execution\n##\n", i);

		// Reset SystemC simulation context
		if (sc_core::sc_curr_simcontext)
			delete sc_core::sc_curr_simcontext;
		sc_core::sc_curr_simcontext = NULL;

		if ((ret = sc_core::sc_elab_and_sim(argc, argv)))
			return ret;
	}

        // …
}

I am aware that this code relies on specific implementation details of the sc_get_curr_simcontext() function and is thus not conforming to IEEE Std 1666-2001. In fact, the code above only works because sc_get_curr_simcontext() currently creates a new simulation context if the global sc_core::sc_curr_simcontext is NULL:

systemc/src/sysc/kernel/sc_simcontext.h

Lines 420 to 423 in c81e0d9

if( sc_curr_simcontext == 0 ) {
sc_default_global_context = new sc_simcontext;
sc_curr_simcontext = sc_default_global_context;
}

Apart from relying on implementation-defined behaviour, an additional problem with the code above is that SystemC contains global variables which are independent of the simulation context. Some of these variables even hold references to the simulation context which cannot be updated and thus cause a double free on program termination. One example of such variables is the sc_event::none variable which was added in 55da81d:

systemc/src/sysc/kernel/sc_event.h

Lines 297 to 298 in 55da81d

// never notified event
static const sc_event none;

As sc_event::none is a global variable, it is initialized on program startup and holds a reference to a simulation context (created through the invocation of sc_get_curr_simcontext() by the sc_event constructor). This reference is not updated by the loop from above, in fact in cannot be updated as the m_simc member of sc_event is private. An additional global variable with the same problem is sc_process_handle::non_event:

systemc/src/sysc/kernel/sc_process.cpp

Line 50 in f567c9c

sc_event sc_process_handle::non_event( sc_event::kernel_event );

I am currently considering working on a modified version of SystemC which addresses issue related to such global variables. I created this issue to figure out if there is an interest in reducing global state in SystemC and if patches in this regard would be accepted.
@mattgately
Copy link

I agree that this is a useful feature request. I am also doing something where I reset the simcontext to enable rerunning of the simulation kernel.

nmeum added a commit to nmeum/systemc that referenced this issue Sep 24, 2020
@nmeum
sc_event::none - convert from global variable to function
1292c02 
The sc_event::none variable was introduced in
55da81d. The problem with the
introduced implementation is that the created event contains a
reference to the simulation context. This simulation context is created
by the sc_event constructor through the invocation of
sc_get_curr_simcontext() on program startup.

This commit proposes a change which tracks the none event as part of the
simulation context. The change converts sc_event::none to a static
member function which returns a reference to the m_none member of the
current simulation context. This eases reset of the simulation context
as discussed in accellera-official#8.

Neither sc_event::none nor sc_get_curr_simcontext() are currently
mandated by IEEE Std 1666-2011. For this reason, this change should not
affected existing standard-conforming SystemC programs.

Signed-off-by: Sören Tempel <tempel@uni-bremen.de>
nmeum added a commit to nmeum/systemc that referenced this issue Sep 24, 2020
@nmeum
sc_process_handle::non_event - convert from global variable to function
86b0514 
Similar to sc_event::none, sc_process_handle::non_event is a global
sc_event which holds a reference to the simulation context. Thereby
complicating reset of this context as discussed in accellera-official#8. This commit
couples the sc_process_handle::non_event with the simulation context,
thereby easing resets of the latter.

At the time of writing, it is unclear to me whether it is strictly
necessary to use different sc_event members in sc_simcontext for
sc_event::none and sc_process_handle::non_event.

Signed-off-by: Sören Tempel <tempel@uni-bremen.de>
nmeum added a commit to nmeum/systemc that referenced this issue Sep 24, 2020
@nmeum
sc_process_handle::non_event - convert from global variable to function
fc2f6a5 
Similar to sc_event::none, sc_process_handle::non_event is a global
sc_event which holds a reference to the simulation context. Thereby
complicating reset of this context as discussed in accellera-official#8. This commit
couples the sc_process_handle::non_event with the simulation context,
thereby easing resets of the latter.

At the time of writing, it is unclear to me whether it is strictly
necessary to use different sc_event members in sc_simcontext for
sc_event::none and sc_process_handle::non_event.

Signed-off-by: Sören Tempel <tempel@uni-bremen.de>
@nmeum nmeum mentioned this issue Sep 24, 2020
Closed
@wltr
Copy link

wltr commented Jun 11, 2021

Has there be any progress on this? Not being able to restart the simulation is the biggest limitation of SystemC imho and I'd be very interested in this.

@nmeum
Copy link
Author

nmeum commented Jun 14, 2021

We are using the patches from #9 as part of a SystemC-based symbolic execution engine, there is some room for improvement but the general idea does seem to work.

@AndrewGoodrich
Copy link
Contributor

There are number of global structures that really should be reset when trying to use a "fresh" simulation context. I think we might be better off if we had a reinitialize() method in the sc_simcontext class, that would allow the code to properly reinitialize those global structures.

@AndrewGoodrich AndrewGoodrich mentioned this issue Sep 21, 2021
Closed
4umber pushed a commit to 4umber/systemc that referenced this issue Sep 22, 2021
@nmeum
sc_event::none - convert from global variable to function
049ab11 
The sc_event::none variable was introduced in
55da81d. The problem with the
introduced implementation is that the created event contains a
reference to the simulation context. This simulation context is created
by the sc_event constructor through the invocation of
sc_get_curr_simcontext() on program startup.

This commit proposes a change which tracks the none event as part of the
simulation context. The change converts sc_event::none to a static
member function which returns a reference to the m_none member of the
current simulation context. This eases reset of the simulation context
as discussed in accellera-official#8.

Neither sc_event::none nor sc_get_curr_simcontext() are currently
mandated by IEEE Std 1666-2011. For this reason, this change should not
affected existing standard-conforming SystemC programs.

Signed-off-by: Sören Tempel <tempel@uni-bremen.de>
4umber pushed a commit to 4umber/systemc that referenced this issue Sep 22, 2021
@nmeum
sc_process_handle::non_event - convert from global variable to function
2550860 
Similar to sc_event::none, sc_process_handle::non_event is a global
sc_event which holds a reference to the simulation context. Thereby
complicating reset of this context as discussed in accellera-official#8. This commit
couples the sc_process_handle::non_event with the simulation context,
thereby easing resets of the latter.

At the time of writing, it is unclear to me whether it is strictly
necessary to use different sc_event members in sc_simcontext for
sc_event::none and sc_process_handle::non_event.

Signed-off-by: Sören Tempel <tempel@uni-bremen.de>
@thatTPR
Copy link

thatTPR commented Oct 14, 2021

Is this solved? If not I would appreciate pointers on helping make this feature available.

@maehne
Copy link
Contributor

maehne commented Nov 11, 2021

@AndrewGoodrich: Might you be able to provide some pointers to @thatTPR?

@AndrewGoodrich
Copy link
Contributor

AndrewGoodrich commented Nov 12, 2021 via email

lmailletcontoz pushed a commit that referenced this issue Dec 1, 2022
@Einwich
uninitialized access wartnings removed:
7c8675c 
==33083==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f36df113d73 in sc_dt::vec_cmp(int, unsigned int const*, int,
unsigned int const*)
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/int/sc_nbutils.h:427:3
    #1 0x7f36df113d73 in sc_dt::vec_skip_and_cmp(int, unsigned int
const*, int, unsigned int const*)
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/int/sc_nbutils.h:500:10
    #2 0x7f36df113d73 in sc_dt::compare_unsigned(int, int, int, unsigned
int const*, int, int, int, unsigned int const*, int, int)
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/int/sc_unsigned.cpp:2090:21
    #3 0x7f36df13e1ab in sc_dt::operator==(sc_dt::sc_unsigned const&,
sc_dt::sc_unsigned const&)
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/int/sc_unsigned.cpp:1722:7
    #4 0x7f36df13e1ab in sc_dt::operator!=(sc_dt::sc_unsigned const&,
sc_dt::sc_unsigned const&)
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/int/sc_nbcommon.inc:1930:13
    #5 0x4ab2d4 in sc_main
/builds/coseda/systemc/systemc-regressions/scripts/systemc/datatypes/misc/concat/test07/./test07/test07.cpp:119:5
    #6 0x7f36dec273e5 in sc_elab_and_sim
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/kernel/sc_main_main.cpp:89:18
    #7 0x7f36dec26f38 in main
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/kernel/sc_main.cpp:36:9
    #8 0x7f36dd41a492 in __libc_start_main (/lib64/libc.so.6+0x23492)
    #9 0x42387d in _start
(/builds/coseda/systemc/systemc-regressions/scripts/systemc/datatypes/misc/concat/test07/systemc.exe+0x42387d)

  Uninitialized value was stored to memory at
    #0 0x7f36def86e38 in sc_dt::sc_signed::concat_get_data(unsigned
int*, int) const
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/int/sc_signed.cpp:337:26
    #1 0x4b9fae in sc_dt::sc_concatref::value() const
/opt/systemc-latest/include/sysc/datatypes/misc/sc_concatref.h:265:41
    #2 0x4ab087 in sc_main
/builds/coseda/systemc/systemc-regressions/scripts/systemc/datatypes/misc/concat/test07/./test07/test07.cpp:119:5
    #3 0x7f36dec273e5 in sc_elab_and_sim
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/kernel/sc_main_main.cpp:89:18
    #4 0x7f36dec26f38 in main
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/kernel/sc_main.cpp:36:9
    #5 0x7f36dd41a492 in __libc_start_main (/lib64/libc.so.6+0x23492)

  Uninitialized value was created by a heap allocation
    #0 0x4a8abb in operator new(unsigned long)
(/builds/coseda/systemc/systemc-regressions/scripts/systemc/datatypes/misc/concat/test07/systemc.exe+0x4a8abb)
    #1 0x7f36debf6f3b in sc_core::sc_byte_heap::initialize(unsigned
long) /builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/utils/sc_temporary.h:96:19
    #2 0x7f36debf6f3b in sc_core::sc_byte_heap::sc_byte_heap(unsigned
long) /builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/utils/sc_temporary.h:114:3
    #3 0x7f36debf6f3b in __cxx_global_var_init
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/misc/sc_concatref.cpp:55:30
    #4 0x7f36debf6f3b in _GLOBAL__sub_I_sc_concatref.cpp
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/misc/sc_concatref.cpp
    #5 0x7f36df4ae8b9 in call_init.part.0
(/lib64/ld-linux-x86-64.so.2+0xf8b9)

SUMMARY: MemorySanitizer: use-of-uninitialized-value
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/datatypes/int/sc_nbutils.h:427:3
in sc_dt::vec_cmp(int, unsigned int const*, int, unsigned int const*)
Exiting



==39132==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x6ba6ac in tlm_utils::simple_target_socket_b<ExplicitATTarget,
32u, tlm::tlm_base_protocol_types,
(sc_core::sc_port_policy)0>::bw_process::nb_transport_bw(tlm::tlm_generic_payload&,
tlm::tlm_phase&, sc_core::sc_time&)
/opt/systemc-latest/include/tlm_utils/simple_target_socket.h:157:13
    #1 0x69bf2b in ExplicitATTarget::beginResponse()
/builds/coseda/systemc/systemc-regressions/include/tlm/ExplicitATTarget.h:133:19
    #2 0x7ff92cf43a94 in sc_core::sc_process_b::semantics()
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/kernel/sc_process.h:685:9
    #3 0x7ff92cf43a94 in sc_core::sc_thread_cor_fn(void*)
/builds/coseda/systemc/systemc-latest/objdir/src/sysc/../../../src/sysc/kernel/sc_thread_process.cpp:117:23
    #4 0x7ff92d4be4bc
(/opt/systemc-latest/lib-linux64/libsystemc-2.3.4.so+0x6b74bc)

  Uninitialized value was created by an allocation of 'target7' in the
stack frame of function 'sc_main'
    #0 0x4f7fe0 in sc_main
/builds/coseda/systemc/systemc-regressions/scripts/tlm/bus_dmi/./bus_dmi/bus_dmi.cpp:37

SUMMARY: MemorySanitizer: use-of-uninitialized-value
/opt/systemc-latest/include/tlm_utils/simple_target_socket.h:157:13 in
tlm_utils::simple_target_socket_b<ExplicitATTarget, 32u,
tlm::tlm_base_protocol_types,
(sc_core::sc_port_policy)0>::bw_process::nb_transport_bw(tlm::tlm_generic_payload&,
tlm::tlm_phase&, sc_core::sc_time&)
Exiting
@wz108578656
Copy link

So does this issue haave a proper solution for now? If yes, many appriciate for anybody to show an example on how to multiple restart the simulation.

@maehne
Copy link
Contributor

maehne commented Aug 29, 2023

@AndrewGoodrich: Could you comment on the state in the released SystemC 2.3.4? As far as I can see from its release notes, your mentioned improvements are not yet part of it.

@AndrewGoodrich
Copy link
Contributor

AndrewGoodrich commented Aug 29, 2023 via email

@AmeyaVS
Copy link

AmeyaVS commented Aug 29, 2023

I don't these changes in this repo here:

systemc/src/sysc/communication/sc_interface.cpp

Line 55 in 3974007

return sc_event::none;

@AmeyaVS
Copy link

AmeyaVS commented Aug 29, 2023

Seems like the release process to public release is broken.

@AndrewGoodrich
Copy link
Contributor

AndrewGoodrich commented Aug 29, 2023 via email

@jakobs
Copy link

jakobs commented Sep 27, 2023

Any possibility to raise the issue again in OSCI-wg/systemc? Resetting a simulation state I find quite relevant, also for unit testing implementations.

@jakobs
Copy link

jakobs commented Oct 20, 2023

@AndrewGoodrich would it be possible that you could include the PR for OSCI-wg/systemc #435 in this public repository? It seems cleaner than replicating your changes based on the issue trail available.

@AndrewGoodrich
Copy link
Contributor

AndrewGoodrich commented Oct 21, 2023 via email

@jakobs
Copy link

jakobs commented Oct 21, 2023 •
edited

I have made the changes in a copy of 2.3.4 in a sandbox on my machine, should I just do a push to get the changes into the repository? Thanks, Andy

@AndrewGoodrich I don't know how this public repository relates to the OSCI-wg/systemc internal one, but I would guess the cleanest way would be for you to clone this public repository, push your changes to that clone and open a Pull Request in this repository. Also it would be great if somehow these changes could make it into an official release.
I much appreciate your support!

@AndrewGoodrich
Copy link
Contributor

AndrewGoodrich commented Oct 21, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@jakobs @wltr @AndrewGoodrich @maehne @nmeum @mattgately @AmeyaVS @wz108578656 @thatTPR