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

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

Merged
merged 6 commits into from
Jun 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf308d9
Init each memory region in init script with zero and fix un-freed buffer
jokap11 Aug 8, 2023
9c00cbb
Added new memseg_initelement_ to initialize memsegs in SimpleMemSys (…
jokap11 May 29, 2024
64138b4
Changed initElement to type String | random init= default
jokap11 Jun 12, 2024
03cc16f
Removed function default values for memInit and MemSegment constr
jokap11 Jun 12, 2024
1a7de07
Implemented different InitVal types like string/hex_val/uninit
jokap11 Jun 13, 2024
6ea7bd2
Considered PR feedback (strtol->stoi)| randomRoot arg | alloc infos s…
jokap11 Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions include/etiss/SimpleMemSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ class MemSegment
/// @param mem Pre-allocated Memory (not overwritten with initString)
/// @param initString String for initialization with imple_mem_system.memseg_initelement_ attr/ If not specified random value allocation
MemSegment(etiss::uint64 start_addr, etiss::uint64 size, access_t mode, const std::string name,
etiss::uint8 *mem = nullptr, std::string initString)
etiss::uint8 *mem = nullptr, std::string initString = "", bool InitEleSet = false)
: name_(name), start_addr_(start_addr), end_addr_(start_addr + size - 1), size_(size), mode_(mode)
{
if (mem)
Expand All @@ -107,7 +107,8 @@ class MemSegment
else
{
mem_ = new etiss::uint8[size];
memInit(initString);
if (InitEleSet)
memInit(initString);
self_allocated_ = true;
}
}
Expand All @@ -118,11 +119,30 @@ class MemSegment
static std::default_random_engine generator{ static_cast<uint64_t>(0) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

std::uniform_int_distribution<int> random_char_{ 0, 255 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could move these lines to where they are actually needed


if (initString == "")
if (initString.find("0x") == 0)
{
initString.erase(initString.begin(),initString.begin()+2);
std::stringstream mem_msg;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mem_msg is unused here

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

char** endPtr = nullptr;
errno = 0;
uint8_t hexVal = static_cast<uint8_t>(strtol(dataPtr, endPtr, 16));
mem_[i] = hexVal;

if (errno != 0){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jokap11 jokap11 Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
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)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did the randomizing go?

}
}
else
Expand Down
22 changes: 16 additions & 6 deletions src/SimpleMemSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ void SimpleMemSystem::load_segments() {

ss << "simple_mem_system.memseg_initelement_" << std::setw(2) << std::setfill('0') << i;
std::string initString = etiss::cfg().get<std::string>(ss.str(), "");
bool initEleSet = etiss::cfg().isSet(ss.str());
std::stringstream().swap(ss);

ss << "simple_mem_system.memseg_image_" << std::setw(2) << std::setfill('0') << i;
Expand Down Expand Up @@ -153,16 +154,25 @@ void SimpleMemSystem::load_segments() {

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 (initString != "") {
mem_msg << "The memory segment " << i << " is initialized with 0x" << std::hex << length << " elements with value: " << initString;
etiss::log(etiss::INFO, mem_msg.str());
} 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
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: keep formatting consistent

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());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jokap11 jokap11 Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, i should not be necessary to keep here.



auto mseg = std::make_unique<MemSegment>(origin, length, static_cast<MemSegment::access_t>(access), sname.str(), buf, initString);
auto mseg = std::make_unique<MemSegment>(origin, length, static_cast<MemSegment::access_t>(access), sname.str(), buf, initString, initEleSet);
add_memsegment(mseg, buf, fsize);
delete[] buf;
}
Expand Down