search

tum-ei-eda / etiss

Extendable Translating Instruction Set Simulator
https://tum-ei-eda.github.io/etiss/
Other
29 stars 36 forks source link

Init each memory region in init script with zero and fix un-freed buffer #138

Closed jokap11 closed 2 months ago

jokap11 commented 1 year ago

Delete Buffer in loadsegments function Initialize memory region with 0 if no image existent

JoGei commented 1 year ago

Hi, makes sense to have a set to 0 feature for memory segments.

Just one concern here from my side. Generating an empty temporary buffer with implicit "set to zero init" to overwrite the memsegment contents through load seems a bit inefficient. Maybe we should instead let the MemSegment Constructor do it by adding an optional "init_zero" parameter.

andrewstevens-infineon commented 1 year ago

Remark: Adding magic 0-initialization memory in models of HW is a recipe for hiding lots of nasty latent bugs that the bite you when you move to a physical system.

If you initialize modelled memory initialize to random garbage or our old friend 0xdeadbeef .

0 is the worst-case as it likely to make a lot of broken SW “ nearly work” as it typically represents “empty” or nullptr. Proper initialization is the job of the hosted OS / runtime libs etc.

Cheers,

            Andrew

From: JoGei @.> Sent: Wednesday, August 9, 2023 9:59 AM To: tum-ei-eda/etiss @.> Cc: Subscribed @.***> Subject: Re: [tum-ei-eda/etiss] Init each memory region in init script with zero and fix un-freed buffer (PR #138)

Caution: This e-mail originated outside Infineon Technologies. Do not click on links or open attachments unless you validate it is safehttps://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx.

Hi, makes sense to have a set to 0 feature for memory segments.

Just one concern here from my side. Generating an empty temporary buffer with implicit "set to zero init" to overwrite the memsegment contents through load seems a bit inefficient. Maybe we should instead let the MemSegment Constructorhttps://github.com/tum-ei-eda/etiss/blob/0d215c87a794a7979f4d728deeeeee0408b8f169/include/etiss/SimpleMemSystem.h#L91-L104 do it by adding an optional "init_zero" parameter.

— Reply to this email directly, view it on GitHubhttps://github.com/tum-ei-eda/etiss/pull/138#issuecomment-1670854156, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AEFJSID6BRLXVAQZH7AJVWDXUM7NPANCNFSM6AAAAAA3IGH3YU. You are receiving this because you are subscribed to this thread.Message ID: @.**@.>>

JoGei commented 1 year ago

Hi Andrew,

Yes, totally agree! Then we should maybe add a config based init type for memory segments like we use for length and origin: simple_mem_system.memseg_initelement_<nr>=<integer-literal> so for example ...

and per default randomize, i.e., no .._initelement given, like we normally do it in the ETISS-SystemC Memory

Regards,

Johannes

jokap11 commented 3 months ago

Considered your feedback @JoGei @andrewstevens-infineon! The default case (simple_mem_system.memseginitelement =0) initializes each MemSeg with randomized values Otherwise, a specific uint8_t value is set to all elements in uint8_t* Array (set by new simple_mem_system.memseginitelement attribute)

PhilippvK commented 3 months ago

Looks good for me. @wysiwyng @JoGei what do you think?

JoGei commented 2 months ago

Looks good for me. @wysiwyng @JoGei what do you think?

IMO, mostly fine, but it should be documented somewhere what the default behavior is.

If you randomize all program memory per default, users might experience unpredictable behavior when launching bad ELFs. E.g., getting different exceptions because of non-deterministic setups between runs.

Considered your feedback @JoGei @andrewstevens-infineon! The default case (simple_mem_system.memseginitelement =0) initializes each MemSeg with randomized values Otherwise, a specific uint8_t value is set to all elements in uint8_t* Array (set by new simple_mem_system.memseginitelement attribute)

With simple_mem_system.memseg_initelement_ =0, I would expect a "set to zero-value" for all memseg elements, not randomize all. Is it possible to parse a string literal first, e.g., simple_mem_system.memseg_initelement_ ="random"? Because right now, we could not set the set value to "0" because it would be interpreted as "random"?

wysiwyng commented 2 months ago

@JoGei we can't both parse a configuration value as integer and string, at least not without considerable hackery. I propose treating simple_mem_system.memseg_initelement_XX absent as "randomize", and all numerical values as "initialize to this value".

A workaround to this would be to treat simple_mem_system.memseg_initelement_XX as a string in any case, and then parse different options out of this string in C++. I.e. simple_mem_system.memseg_initelement_XX missing -> don't initialize at all simple_mem_system.memseg_initelement_XX = "random" -> randomize, simple_mem_system.memseg_initelement_XX = 0xdeadbeef" -> apply pattern repeated to memory.

wysiwyng commented 2 months ago

Lastly, I agree with @JoGei that this feature should be documented somewhere. Where that is I don't really know, as the previous changes to SimpleMemSystem are also only documented in the pull request that added them.

jokap11 commented 2 months ago

@wysiwyng @JoGei @PhilippvK Thank you for your feedback: I changed the simple_mem_system.memseg_initelement_XX element to a string type. The default behavior behavior (not specified attr) allocates a randomized MemSegment. (Documented with Doxygen at the MemSegment Constructor) Otherwise the string is concatenated for the whole MemSegment - The tail is cut to prohibit overflow.

wysiwyng commented 2 months ago

Still missing a case to "not initialize anything" when simple_mem_system.memseg_initelement_XX is not present in the config. You can use etiss::cfg().isSet to check whether a value is present in the configuration. Looks good otherwise.

jokap11 commented 2 months ago

Still missing a case to "not initialize anything" when simple_mem_system.memseg_initelement_XX is not present in the config. You can use etiss::cfg().isSet to check whether a value is present in the configuration. Looks good otherwise.

I assumed that simple_mem_system.memseg_initelement_XX="" is no valid option anyways, that's why the default case in https://github.com/jokap11/etiss/blob/initSimpleMemRegion/src/SimpleMemSystem.cpp#L101 ="" is enough to filter this behavior. Or am I missing sth? memseg_image_ filters in a similar fashion: https://github.com/jokap11/etiss/blob/master/src/SimpleMemSystem.cpp#L134

wysiwyng commented 2 months ago

I assumed that simple_mem_system.memseg_initelement_XX="" is no valid option anyways

As far as I know the ini parser of ETISS accepts that input. What I mean is the case when the configuration file does not contain the key simple_mem_system.memseg_initelement_XX at all. I see the issue however, then the empty string would mean "random". I am also not quite happy with taking the string's characters verbatim to initialize the memory region, you would have to put binary into your configuration file. I think the possible input for simple_mem_system.memseg_initelement_XX should be trifold: 1) nothing or absent: do nothing with the memory segment 2) "random": randomize 3) some hex number, starting with "0x" to make distinguishable: initialize with said number 4) anything else: throw an error for invalid input

memseg_image_ filters in a similar fashion: https://github.com/jokap11/etiss/blob/master/src/SimpleMemSystem.cpp#L134

Yes, because it only needs to differentiate between two cases: something vaguely resembling a file path or empty. We want to differentiate between 3 cases: "Nothing", random, and some set value.