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.

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

Read as many ELF sections as possible. #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

snim2
Copy link
Contributor

@snim2 snim2 commented Jun 29, 2016

This worked with the Revelation simulator, but I have not tested it with the other Pydin simulators.

Reading in the .debug, .comment or .stab sections caused problems with Epiphany ELFs, and I did find an "unknown" section type, so the check for that was necessary. Uncommenting the later code to read in the symbol table worked fine, but I haven't included that in this PR.


if section_name not in ['.sbss', '.bss']:
if (shdr.type == ElfSectionHeader.TYPE_NULL or
section_name == '.comment' or
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to check against section names? I believe these sections have their ALLOC flag unset, and it would be better to use that to prevent them from loading. You can check the ALLOC flags of the sections using readelf -a.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. The section I needed was loader_cfg which, bafflingly, didn't have its ALLOC flag set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm a bit worried this might break other ISAs since they sometimes have other sections which shouldn't be loaded and can corrupt other sections if loaded. Unfortunately Travis might not catch these since assembly tests we use in Travis might not have those ELF sections. I'm not an expert on ELF format but maybe this is a bug in their cross-compiler and ISS? Is this section supposed to be executable or just data?

Copy link
Contributor Author

@snim2 snim2 Jun 29, 2016

Choose a reason for hiding this comment

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

It could well be a bug in their format. I'm pretty sure this location contains some data. I'll try reporting this as an issue on one of the Epiphany repos and see what happens.

adapteva/epiphany-sdk#66

Copy link
Contributor

@berkinilbeyi berkinilbeyi Jun 29, 2016

Choose a reason for hiding this comment

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

I think in the meanwhile, you can hard-code this exception (with a good comment) and still check against FLAGS_ALLOC. Maybe change this with something like:

if (not (shdr.flags & ElfSectionHeader.FLAGS_ALLOC) and
    section_name != 'loader_cfg'):
  continue

@berkinilbeyi
Copy link
Contributor

This approach doesn't seem to work for other ISAs as indicated by Travis. I'll try to figure out how to do this correctly one of these days but haven't gotten the chance yet. Did this patch fix the issue in Revelation?

@snim2
Copy link
Contributor Author

snim2 commented Jul 27, 2016

I did a bit more digging around the e-loader code from Adapteva, and it looks like the .loader_cfg section that Revelation needed (and a few others) are always filled with zeroes in the ELF files. These sections are later populated with "real" values after the ELF file is loaded. The reason for this is that one ELF file can be loaded onto many Epiphany cores, and the user can choose which cores to use with which ELF files at load time, and can place the cores into workgroups. Some information about this configuration is written into .loader_cfg and some other sections.

So, the short story is that I have a work-around for all this in Revelation, so no need to change the Pydgin code upstream. Thanks for checking though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants