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

Choose members of the dynamic tag enum based on the current machine #183

Merged
merged 3 commits into from
Feb 23, 2018

Conversation

rhelmot
Copy link
Contributor

@rhelmot rhelmot commented Feb 22, 2018

closes #175

Adds a testcase and a 35k android marshmallow library (libsoundtrigger.so) for the testcase. The testcase also reuses an existing solaris test binary.

@rhelmot rhelmot changed the title Chose members of the dynamic tag enum based on the current machine Choose members of the dynamic tag enum based on the current machine Feb 22, 2018
@@ -71,7 +71,7 @@ def __init__(self, stream):

self.structs.create_basic_structs()
self.header = self._parse_elf_header()
self.structs.create_advanced_structs(self['e_type'], self['e_machine'])
self.structs.create_advanced_structs(self['e_type'], self['e_machine'], self['e_ident']['EI_OSABI'])
Copy link
Owner

Choose a reason for hiding this comment

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

Please wrap this for 80-col lines

EM_MIPS=ENUM_D_TAG_MIPS,
EM_MIPS_RS3_LE=ENUM_D_TAG_MIPS,
# TODO: add the rest
# solaris doesn't go here because what matters is e_ident[EI_OSABI] == ELFOSABI_SOLARIS
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure I follow this comment. What are you trying to say? It may be obvious that we now have a separate section for the solaris tags, so maybe it's superfluous?

'exe_solaris32_cc.sparc.elf'))
f2 = extract_sunw(os.path.join('test', 'testfiles_for_unittests',
'android_dyntags.elf'))
self.assertEqual(f1, {'DT_SUNW_STRPAD', 'DT_SUNW_LDMACH'})
Copy link
Owner

Choose a reason for hiding this comment

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

Nice testing!

Do you think it would be possible to add readelf-comparison tests for these files?

@eliben
Copy link
Owner

eliben commented Feb 22, 2018

Thanks for the contribution, this looks great overall. I left some (minor) comments in the review.

@rhelmot
Copy link
Contributor Author

rhelmot commented Feb 22, 2018

Fix pushed. I tried running readelf.py on the test files and I actually found a bug with the PR, so that's been resolved (along with the other changes).

However, when I tried adding a readelf-comparison test, it failed extremely badly for a lot of reasons unrelated to this PR, notably the naming of solaris-specific sections and readelf -d outputting a second hunk of section data after the listing of the dynamic tags.

@eliben eliben merged commit 505e5c6 into eliben:master Feb 23, 2018
frederiksdun pushed a commit to frederiksdun/pyelftools that referenced this pull request Jul 16, 2018
…liben#183)

* Only use processor/os specific dynamic tags if that processor or os is in use

* Add testcase for machine-specific dynamic tags

* Clarify layout of ENUM_D_TAG
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.

Only use os- and processor- specific enum values on the appropriate os and processor
2 participants