-
Notifications
You must be signed in to change notification settings - Fork 485
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.
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
add xrt::elf raw elf data constructor #7963
Conversation
HimanshuChoudhary-Xilinx - is not a collaborator |
Can one of the admins verify this patch? |
HimanshuChoudhary-Xilinx - is not a collaborator |
HimanshuChoudhary-Xilinx - is not a collaborator |
ok to test |
ok to test/retest is no longer working. We need to go to actions and run all jobs again. Started run just now |
{ | ||
const std::string elf_string(data.begin(), data.end()); | ||
std::istringstream elf_stream(elf_string); | ||
if (!m_elf.load(elf_stream)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are copying the vector here. Can we write a simple adapter for std::vector to make it look like std::istream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
elf_impl(const std::vector<char>& data) | ||
{ | ||
std::istringstream elf_stream; | ||
elf_stream.rdbuf()->pubsetbuf(const_cast<char*>(data.data()), data.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The const_cast looks evil here. Can you add a comment why we are doing it and the fact that elf_stream is input stream (read only) so it cannot accidentally change data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since data is const type so data.data() return const char*
https://cplusplus.com/reference/vector/vector/data/
If the vector object is const-qualified, the function returns a pointer to const value_type. Otherwise, it returns a pointer to value_type.
while
streambuf* pubsetbuf (char* s, streamsize n);
https://cplusplus.com/reference/streambuf/streambuf/pubsetbuf/
* | ||
*/ | ||
XRT_API_EXPORT | ||
elf(const std::vector<char>& data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A constructor from std::vector isn't great IMO. What if data is not held in a vector? A std::span would be better, but that's c++20. I would opt for const char*, size_t
but you are still faced with the problem of how to create the istream without copying the in-memory buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stsoe
Updated the PR with changes discussed offline.
Signed-off-by: HimanshuChoudhary-Xilinx <[email protected]>
Problem solved by the commit
Added new xrt::elf constructor to take in-memory buffer as input
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
NA
How problem was solved, alternative solutions (if any) and why they were rejected
added new constructor to take memory elf buffer
Risks (if any) associated the changes in the commit
None
What has been tested and how, request additional testing if necessary
taken multiple efl and check we are able to get correct xrt::elf object
Documentation impact (if any)
Yes, we have to update XRT doc