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

Jump to bottom

arm32 unwind fix #1102

Merged
merged 4 commits into from
Oct 13, 2016
Merged

arm32 unwind fix #1102

merged 4 commits into from
Oct 13, 2016

Conversation

jforissier
Copy link
Contributor

No description provided.

@jforissier jforissier mentioned this pull request Oct 10, 2016
Closed
jenswi-linaro
jenswi-linaro reviewed Oct 10, 2016
View reviewed changes
core/arch/arm/kernel/unwind_arm32.c
@@ -367,10 +367,11 @@ void print_stack(int level)
{
struct unwind_state state;

memset(&state, 0, sizeof(state));
asm volatile("stmia %0, {r0-r12}" :: "r" (state.registers) : "memory");
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to depend on how the compiler uses the registers.

@jforissier
Copy link
Contributor Author

Updated.

etienne-lms
etienne-lms reviewed Oct 11, 2016
View reviewed changes
core/arch/arm/kernel/unwind_arm32.c
memset(&state, 0, sizeof(state));
memset(state.registers, 0, sizeof(state.registers));
/* r7: Thumb-style frame pointer */
state.registers[7] = read_r7();
Copy link
Contributor

Choose a reason for hiding this comment

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

for consistency, could you use a R7 label defined next to FP, SP and friends above ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not for two reasons: (1) it does not improve legibility and (2) the above code is mostly copied from FreeBSD so the less we change it, the better.

etienne-lms
etienne-lms reviewed Oct 11, 2016
View reviewed changes
core/arch/arm/kernel/unwind_arm32.c
@@ -367,10 +367,14 @@ void print_stack(int level)
{
struct unwind_state state;

memset(&state, 0, sizeof(state));
memset(state.registers, 0, sizeof(state.registers));
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this is a stupid question: why zero-ing only the registers sub structure ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because, that's the only part that contains input arguments. The rest is used internally by the unwinding code, so it doesn't look like it is the responsibility of the caller to initialize it (To which value BTW? Is zero acceptable for all fields?).

etienne-lms
etienne-lms suggested changes Oct 11, 2016
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

minor comments

@etienne-lms
Copy link
Contributor

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

@jenswi-linaro
Copy link
Contributor

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

@jforissier
arm32: unwind: fix incorrect return status
7cd4334 
After the unwind code was imported from FreeBSD sources, it was slightly
modified to invert some logic. One return slipped through.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
@jforissier
arm32: unwind: convert int to bool
9c5e2f8 
The return status of unwind_tab() is used as a boolean, so change its
type.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
@jforissier
arm32: unwind: mark tee_svc_do_call() with .cantunwind
e386996 
The assembly function tee_svc_do_call() manipulates the stack pointer
but does not use the proper unwind directives when doing so. As a
result, the compiler can't generate proper unwind information. This can
lead to crashes or infinite loops if unwinding is performed at runtime.
Given that there is nothing of much interest below this function, we
simply add a .cantundwind directive to stop unwinding here.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
@jforissier
arm32: unwind: print_stack(): fix unwind_state
18e8c53 
print_stack() must save r7 and r11 in the unwind_state structure. Not
doing so will likely result in a crash dunring unwind.
Register r7 is typically used as a frame pointer by GCC in Thumb2 mode,
while r11 (a.k.a. fp) is the frame pointer in ARM mode.

Also, set PC to the beginning of print_stack() since there's no point
in going further inside the function.

Fixes: OP-TEE#1069
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey)
Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (QEMU)
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
@jforissier jforissier merged commit 18e8c53 into OP-TEE:master Oct 13, 2016
@jforissier jforissier deleted the arm32-unwind-fix branch October 13, 2016 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenswi-linaro jenswi-linaro jenswi-linaro left review comments

@etienne-lms etienne-lms etienne-lms requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jforissier @etienne-lms @jenswi-linaro