Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for ARM exception handler ABI #328

Merged
merged 22 commits into from
Aug 19, 2020
Merged

Conversation

LeadroyaL
Copy link
Contributor

Dwarf and ehabi are two different unwind model. ARM ELF file use .ARM.exidx and .ARM.extab secions to unwind stack trace. The description of data can be shown with command binutils readelf -u filename. Pyelftools doesn't parse these two sections, which is reason for this pull request.

Like DWARFInfo, EHABIInfo is also independent with ELFFile. It just need initialized by a SHT_ARM_EXIDX section.

Also I add argument -au, --arm_unwind in readelf.py. I cannot use -u because this pull request only compatible for ARM.

Referense:
https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ARMEHABIPrinter.h
https://developer.arm.com/documentation/ihi0038/b/
binutils: readelf.c

@eliben
Copy link
Owner

eliben commented Jul 25, 2020

Thank you for the contribution. I'm not much familiar with ARM and its EHABI. CCing some folks who contributed ARM stuff before who could help review this

@frederiksdun @nickdesaulniers

Copy link
Contributor

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

I found the inline references to the source code to LLVM's decoder helpful.

Should the tests have the source file that produced them, as well as comments on how to rebuild the test object files?

Looking at some of the bitwise operations, I'm curious about the perennial question for bi-endian architectures; does this work for big endian (compile with -mbig-endian). If yes; please add little and big endian binaries. If not, well I guess little endian is more standard, and something is better than nothing.

@smithp35 is the expert here.

test/test_ehabi_elf.py Outdated Show resolved Hide resolved
elftools/elf/elffile.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
@LeadroyaL
Copy link
Contributor Author

Should the tests have the source file that produced them, as well as comments on how to rebuild the test object files?

You're right. I'll try to add the testcase from LLVM-readelf and produce them later.

Looking at some of the bitwise operations, I'm curious about the perennial question for bi-endian architectures; does this work for big endian (compile with -mbig-endian). If yes; please add little and big endian binaries. If not, well I guess little endian is more standard, and something is better than nothing.

Sorry I forget to handle endian, I'll fix it later also.

@nickdesaulniers
Copy link
Contributor

The implementation LGTM; just style nits that elftools/ehabi/decoder.py has way too many UpperCamelCased local variable identifiers. I think those should only be used for classes. (Github highlights them in orange). Some methods also use _UpperCamelCase, and I think those should be _snake_case. PEP8 says:

Function names should be lowercase, with words separated by underscores as necessary to improve readability.

elftools/ehabi/ehabiinfo.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/ehabiinfo.py Outdated Show resolved Hide resolved
elftools/ehabi/ehabiinfo.py Outdated Show resolved Hide resolved
elftools/ehabi/ehabiinfo.py Show resolved Hide resolved
elftools/ehabi/ehabiinfo.py Outdated Show resolved Hide resolved
elftools/ehabi/ehabiinfo.py Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
Copy link
Contributor

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

cool LGTM; thanks for all of the work that went into this.

@eliben I think this is good to merge.

elftools/ehabi/constants.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Outdated Show resolved Hide resolved
elftools/ehabi/decoder.py Show resolved Hide resolved
elftools/ehabi/ehabiinfo.py Outdated Show resolved Hide resolved
elftools/ehabi/ehabiinfo.py Outdated Show resolved Hide resolved
elftools/ehabi/ehabiinfo.py Outdated Show resolved Hide resolved
elftools/elf/elffile.py Outdated Show resolved Hide resolved
test/testfiles_for_unittests/arm_exidx_test.cpp Outdated Show resolved Hide resolved
@eliben
Copy link
Owner

eliben commented Jul 31, 2020

Just a note that I'll be offline for a week or so, so this gives you plenty of time to address the (mostly minor) comments. I'll re-review and merge when I'm back

1. delete all print in deep library
2. PEP8: end file with newline, 80 chars limit
3. use namedtuple
4. more comment
5. visit `attr` directly rather than `.header[attr]`
@LeadroyaL LeadroyaL requested a review from eliben August 18, 2020 06:39
Copy link
Owner

@eliben eliben left a comment

Choose a reason for hiding this comment

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

Thanks!

@eliben eliben merged commit ee0face into eliben:master Aug 19, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants