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

jokap11 · 2023-08-08T09:42:34Z

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

JoGei · 2023-08-09T07:59:08Z

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 · 2023-08-09T08:07:57Z

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 safe<https://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 Constructor<https://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 GitHub<#138 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEFJSID6BRLXVAQZH7AJVWDXUM7NPANCNFSM6AAAAAA3IGH3YU>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.******@***.***>>

JoGei · 2023-08-09T08:34:11Z

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 ...

simple_mem_system.memseg_initelement_00=0xdeadbeef -> "specific integer literal"
simple_mem_system.memseg_initelement_00=-1 -> "all set"
simple_mem_system.memseg_initelement_00=0 -> "all reset"

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

Regards,

Johannes

…Default init=random)

jokap11 · 2024-05-29T12:19:37Z

Considered your feedback @JoGei @andrewstevens-infineon!
The default case (simple_mem_system.memseg_initelement_ =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.memseg_initelement_ attribute)

PhilippvK · 2024-06-06T07:35:32Z

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

JoGei · 2024-06-10T13:49:14Z

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.memseg_initelement_ =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.memseg_initelement_ 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 · 2024-06-11T14:07:27Z

@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

nitpicking at minor code quality things

wysiwyng · 2024-06-11T14:08:01Z

.github/workflows/ci.yml

Keep this file out of this PR, it has nothing to do with the memory init stuff

wysiwyng · 2024-06-11T14:08:36Z

include/etiss/SimpleMemSystem.h

+        // std::stringstream mem_msg;
+        // mem_msg << "This memory segment "  <<  " is initialized with 0x" << std::hex << size_ << " bytes! \n";
+        // for (etiss::uint64 i = 0; i < size_; ++i)
+        // {
+        //     mem_msg << static_cast<uint16_t>( mem_[i]) << ":";
+        // }
+        // etiss::log(etiss::INFO, mem_msg.str());


can be removed

wysiwyng · 2024-06-11T14:09:23Z

src/SimpleMemSystem.cpp

+
+                mem_msg << "The memory segment " <<  i  <<  " is initialized with 0x" << std::hex << length << " bytes from input_image !";
+                etiss::log(etiss::INFO, mem_msg.str());
+            }else if (initVal != 0){


keep code formatting consistent

wysiwyng · 2024-06-11T14:09:26Z

src/SimpleMemSystem.cpp

+            }else if (initVal != 0){
+                mem_msg << "The memory segment " <<  i  <<  " is initialized with 0x" << std::hex << length << " elements with value: " << static_cast<uint16_t>(initVal);
+                etiss::log(etiss::INFO, mem_msg.str());
+            }else{


keep code formatting consistent

wysiwyng · 2024-06-11T14:11:07Z

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 · 2024-06-12T12:15:07Z

@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.

JoGei

Not sure about the std::string param= 0 default arguments.

JoGei · 2024-06-12T12:22:47Z

include/etiss/SimpleMemSystem.h

            self_allocated_ = true;
        }
    }

    // Can be overwritten afterwards with load_elf
-    void memInit(uint8_t initVal = 0)
+    void memInit(std::string initString = 0)


std::string default argument should be = "" or removed since it now will be managed by etiss::initialize

JoGei · 2024-06-12T12:22:52Z

include/etiss/SimpleMemSystem.h

    MemSegment(etiss::uint64 start_addr, etiss::uint64 size, access_t mode, const std::string name,
-               etiss::uint8 *mem = nullptr, uint8_t initVal = 0)
+               etiss::uint8 *mem = nullptr, std::string initString = 0)


std::string default argument should be = "" or removed since it now will be managed by etiss::initialize

wysiwyng · 2024-06-12T12:39:06Z

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 · 2024-06-12T12:49:03Z

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 · 2024-06-12T13:15:55Z

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:

nothing or absent: do nothing with the memory segment
"random": randomize
some hex number, starting with "0x" to make distinguishable: initialize with said number
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.

wysiwyng · 2024-06-13T13:34:47Z

src/SimpleMemSystem.cpp

+                }
+                else
+                {


minor: keep formatting consistent

wysiwyng · 2024-06-13T13:41:51Z

include/etiss/SimpleMemSystem.h

+                uint8_t hexVal = static_cast<uint8_t>(strtol(dataPtr, endPtr, 16));
+                mem_[i] =  hexVal;
+
+                if (errno != 0){


errno != 0 only indicates an out-of-range condition for strtol, not an invalid string, see: https://en.cppreference.com/w/c/string/byte/strtol. To avoid this, use std::stoi: https://en.cppreference.com/w/cpp/string/basic_string/stol and handle the thrown exception(s).

Thank you for clarifying. I was a bit confused after the errno=EINVAL option was not triggered with a false "hex val string": e.g 0xDEADBEEFh -> 0xDEADBEEF0 instead of leading to an errno!=0.
A weird thing though is that vscode leads me to this linux header:
https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/errno-base.h#L26
I assume that my work folder probably accesses unused files

wysiwyng · 2024-06-13T13:42:17Z

include/etiss/SimpleMemSystem.h

+                        etiss::log(etiss::WARNING, "Hex Value MemSegment input is erronous (typo?)");
+                        throw "MemSegmentInit for hex value is errnounous";


use etiss::FATALERROR to exit ETISS without having to resort to throw

wysiwyng · 2024-06-13T13:44:38Z

include/etiss/SimpleMemSystem.h

            for (etiss::uint64 i = 0; i < size_; ++i)
            {
-                mem_[i] = random_char_(generator);
+                const char* dataPtr = initString.substr(i%initString.length(),1).c_str();


reading one hex character at a time will give you only 4 bits, you need 8

wysiwyng · 2024-06-13T14:00:57Z

include/etiss/SimpleMemSystem.h

        {
+            initString.erase(initString.begin(),initString.begin()+2);
+            std::stringstream mem_msg;


mem_msg is unused here

wysiwyng · 2024-06-13T14:10:08Z

include/etiss/SimpleMemSystem.h

+        else if (initString.find("random") == 0 || initString.find("RANDOM") == 0)
+        {
+            const char* data = initString.c_str();
+            for (etiss::uint64 i = 0; i < size_; ++i)
+            {
+                mem_[i] = data[i%strlen(data)];


where did the randomizing go?

wysiwyng · 2024-06-13T14:10:39Z

include/etiss/SimpleMemSystem.h

        static std::default_random_engine generator{ static_cast<uint64_t>(0) };
        std::uniform_int_distribution<int> random_char_{ 0, 255 };


could move these lines to where they are actually needed

wysiwyng · 2024-06-13T14:13:42Z

src/SimpleMemSystem.cpp

+            } else if (initEleSet) {
+                if (initString.find("random") == 0 || initString.find("RANDOM")== 0) {
+                    mem_msg << "The memory segment " <<  i  <<  " is initialized with 0x" << std::hex << length << " random bytes !";
+                    etiss::log(etiss::INFO, mem_msg.str());
+                } else if (initString.find("0x") == 0) {
+                    mem_msg << "The memory segment " <<  i  <<  " is initialized with 0x" << std::hex << length << " elements with hex value: " << initString;
+                    etiss::log(etiss::INFO, mem_msg.str());
+                }
+                else
+                {
+                    mem_msg << "The memory segment " <<  i  <<  " is initialized with 0x" << std::hex << length << " elements with the string: " << initString;
+                    etiss::log(etiss::INFO, mem_msg.str());
+                }
            } else {
-                mem_msg << "The memory segment " <<  i  <<  " is initialized with 0x" << std::hex << length << " random values !";
+                mem_msg << "The memory segment " <<  i  <<  " is allocated uninitialized with length 0x" << std::hex << length << " !";
                etiss::log(etiss::INFO, mem_msg.str());
            }


all these conditionals do is print debug messages, move the messages to MemSegment() where these differentiations have to be made anyways

Well I lacked the local i var. Therfore I let this on this level. Otherwise I will remove just the i var since it is quite clear which MemSegment is initialized with which values.

agreed, i should not be necessary to keep here.

PhilippvK · 2024-06-13T15:34:00Z

include/etiss/SimpleMemSystem.h

+    // Can be overwritten afterwards with load_elf
+    void memInit(std::string initString)
+    {
+        static std::default_random_engine generator{ static_cast<uint64_t>(0) };


I would prefer having the seed (optionally) configurable via INI settings. Otherwise it would be quite hard to reproduce bugs,…

…hift

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

cf308d9

Added new memseg_initelement_ to initialize memsegs in SimpleMemSys (…

9c00cbb

…Default init=random)

wysiwyng requested changes Jun 11, 2024

View reviewed changes

jokap11 force-pushed the initSimpleMemRegion branch from 023b7e3 to 9c00cbb Compare June 12, 2024 10:27

Changed initElement to type String | random init= default

64138b4

JoGei requested changes Jun 12, 2024

View reviewed changes

Removed function default values for memInit and MemSegment constr

03cc16f

Implemented different InitVal types like string/hex_val/uninit

1a7de07

jokap11 requested a review from wysiwyng June 13, 2024 13:58

wysiwyng requested changes Jun 13, 2024

View reviewed changes

PhilippvK reviewed Jun 13, 2024

View reviewed changes

Considered PR feedback (strtol->stoi)| randomRoot arg | alloc infos s…

6ea7bd2

…hift

jokap11 requested review from wysiwyng, JoGei and PhilippvK June 14, 2024 09:18

wysiwyng merged commit 6af4793 into tum-ei-eda:master Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jokap11 commented Aug 8, 2023

JoGei commented Aug 9, 2023

andrewstevens-infineon commented Aug 9, 2023 via email

JoGei commented Aug 9, 2023

jokap11 commented May 29, 2024

PhilippvK commented Jun 6, 2024

JoGei commented Jun 10, 2024

wysiwyng commented Jun 11, 2024

wysiwyng left a comment

wysiwyng Jun 11, 2024

wysiwyng Jun 11, 2024

wysiwyng Jun 11, 2024

wysiwyng Jun 11, 2024

wysiwyng commented Jun 11, 2024

jokap11 commented Jun 12, 2024

JoGei left a comment

JoGei Jun 12, 2024

JoGei Jun 12, 2024

wysiwyng commented Jun 12, 2024

jokap11 commented Jun 12, 2024 •

edited

Loading

wysiwyng commented Jun 12, 2024

wysiwyng Jun 13, 2024

wysiwyng Jun 13, 2024

jokap11 Jun 13, 2024 •

edited

Loading

wysiwyng Jun 13, 2024

wysiwyng Jun 13, 2024

wysiwyng Jun 13, 2024

wysiwyng Jun 13, 2024

wysiwyng Jun 13, 2024

wysiwyng Jun 13, 2024

jokap11 Jun 13, 2024 •

edited

Loading

wysiwyng Jun 13, 2024

PhilippvK Jun 13, 2024

		etiss::log(etiss::WARNING, "Hex Value MemSegment input is erronous (typo?)");
		throw "MemSegmentInit for hex value is errnounous";

		static std::default_random_engine generator{ static_cast<uint64_t>(0) };
		std::uniform_int_distribution<int> random_char_{ 0, 255 };

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

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

Conversation

jokap11 commented Aug 8, 2023

JoGei commented Aug 9, 2023

andrewstevens-infineon commented Aug 9, 2023 via email

JoGei commented Aug 9, 2023

jokap11 commented May 29, 2024

PhilippvK commented Jun 6, 2024

JoGei commented Jun 10, 2024

wysiwyng commented Jun 11, 2024

wysiwyng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wysiwyng commented Jun 11, 2024

jokap11 commented Jun 12, 2024

JoGei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wysiwyng commented Jun 12, 2024

jokap11 commented Jun 12, 2024 • edited Loading

wysiwyng commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokap11 Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokap11 Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokap11 commented Jun 12, 2024 •

edited

Loading

jokap11 Jun 13, 2024 •

edited

Loading

jokap11 Jun 13, 2024 •

edited

Loading