Page MenuHomePhabricator

Fix handling of the arm IT instruction in the unwinder
ClosedPublic

Authored by tberghammer on Feb 2 2016, 10:19 AM.

Details

Summary

Fix handling of the arm IT instruction in the unwinder

The IT instruction can specify condition code for up to 4 consecutive
instruction and it is used quite often by clang in epilogues causing
an issue when trying to unwind from locations covered by the IT
instruction and for locations immediately after the IT instruction.

Changes made to fix it:

  • Introduce the concept of conditional instruction block what is a list of consecutive instructions with the same condition. We update the unwind information during the conditional instruction block and when we reach the end of it (first instruction with a different condition) then we restore the unwind information we had before the condition.
  • Fix a bug in the ARM instruction emulator where neither PC nor the ITSTATE was advanced when we reached an instruction what we can't decode.

After the change we have no regression on android-arm running the
regular test suit and TestStandardUnwind also passes when running it
with clang as the compiler (previously it failed on an IT instruction).

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer retitled this revision from to Fix handling of the arm IT instruction in the unwinder.
tberghammer updated this object.
tberghammer added a subscriber: lldb-commits.
clayborg requested changes to this revision.Feb 2 2016, 10:42 AM
clayborg edited edge metadata.

It would be nice to not pick UINT32_MAX for the unconditional condition and let each emulator picks it desired values. See inlined comments. Let me know what you think.

include/lldb/Core/EmulateInstruction.h
406–410 ↗(On Diff #46672)

Add:

virtual uint32_t
GetUnconditionalCondition () { return 0; };

And change GetInstructionCondition to use the above function:

virtual uint32_t
GetInstructionCondition() { return GetUnconditionalCondition (); }

See inline comments below for more info...

source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
130 ↗(On Diff #46672)

Maybe require the instruction emulator to give us an unconditional value here?

const uint32_t unconditional_condition = m_inst_emulator_ap->GetUnconditionalCondition();
uint32_t last_condition = unconditional_condition;

UINT32_MAX follows the ARM way of thinking, but other chips might have their own notion where zero is the unconditional value. It would be nice to not pick any default values for all instruction emulators.

163 ↗(On Diff #46672)

Change to:

if (m_inst_emulator_ap->GetInstructionCondition() != unconditional_condition &&
175 ↗(On Diff #46672)

Change to:

if (last_condition != unconditional_condition)
This revision now requires changes to proceed.Feb 2 2016, 10:42 AM
labath added a subscriber: labath.Feb 2 2016, 10:59 AM
labath added inline comments.
include/lldb/Core/EmulateInstruction.h
406–410 ↗(On Diff #46672)

How about

virtual llvm::Optional<uint32_t>
GetInstructionCondition() { return llvm::NoneType; }

?

I like that llvm::Optional<uint32_t> idea!

UINT32_MAX is a kind of a random value what is most likely won't be used on any architecture as a condition code (I can't imagine having so much different condition flags) and my current intention is to map the unconditional value to into it as it is already done on ARM where the unconditional value is 0b1110 and 0b1111.

I don't really like the idea of introducing a new GetUnconditionalCondition() method because I think it over complicates the API in a situation where it is not necessary. I would better prefer to create a static constant for the unconditional condition (with setting it to UINT32_MAX) so the implementation is a bit cleaner. If we really want to make a very clean interface then I would suggest to introduce a new "Condition" class with a virtual equality operator and a virtual isUnconditional function and then we can make GetInstructionCondition return a pointer to an instance so we don't restrict the storage format for the architectures to a uint32_t. I think this would be the most general API but I also feel it would be a massive over-engineering.

For llvm::Optional<T> the main issue is that (non) equality operator is intentionally not defined for llvm::Optional<T> (I don't know why) what would make UnwindAssemblyInstEmulation.cpp:161 very strange. Also debugging code containing llvm data types is generally pretty complicated.

All in all I would suggest to use a uint32_t/uint64_t for the condition code with UINT32_MAX/UINT64_MAX meaning unconditional and I propose the introduction of a new public/protected static constant on EmulateInstruction what represents the unconditional value. This way the implementation is cleaner because will be obvious when we are comparing against an unconditional condition with keeping the API simple.

What do you think?

tberghammer updated this revision to Diff 46764.Feb 3 2016, 3:54 AM
tberghammer edited edge metadata.

Create a typedef for the condition type and a static const value for the unconditional condition.

labath added a comment.Feb 3 2016, 8:46 AM

I agree that we should not over-engineer things. I'll leave the decision up to others...

clayborg requested changes to this revision.Feb 3 2016, 10:31 AM
clayborg edited edge metadata.

Yes, it was probably too complex. My main objection was the use of the UINT32_MAX as a magic number. Your UnconditionalCondition solution clears this up.

include/lldb/Core/EmulateInstruction.h
390 ↗(On Diff #46764)

It is nice to tell when something is a constant (prefixed with "k"), a member variable (prefixed with "m_"), typenames are camel case, variables should be lower cased with underscores. That is our current coding convention.

I would like to see this be

static const InstructionCondition k_unconditional_condition = UINT32_MAX;
412 ↗(On Diff #46764)

k_unconditional_condition

source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
13653 ↗(On Diff #46764)

Do you want to use UnconditionalCondition (or k_unconditional_condition) instead of UINT32_MAX here?

13654 ↗(On Diff #46764)

k_unconditional_condition

source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
129 ↗(On Diff #46764)

k_unconditional_condition

This revision now requires changes to proceed.Feb 3 2016, 10:31 AM
tberghammer added inline comments.Feb 4 2016, 3:41 AM
include/lldb/Core/EmulateInstruction.h
390 ↗(On Diff #46764)

It is a public, static member variable so I think we should use CamelCase and not snake_case and I don't see where I can add a k prefix into it (KInstructionCondition?) Also as (almost) all static member is cons inside LLDB I don't think it will be confusing

Jason: Can you take at the change in the unwinding logic?

source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
13653 ↗(On Diff #46764)

No, here we are inside EmulateInstructionARM. CurrentCond returns UINT32_MAX as a failure value what we want to treat as unconditional (as we can't do anything better). Changing CurrentCond to return UnconditionalCondition in case of a failure won't make any sense either.

clayborg accepted this revision.Feb 9 2016, 12:55 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Feb 9 2016, 12:55 PM
This revision was automatically updated to reflect the committed changes.