[Lldb-commits] [PATCH] D16814: Fix handling of the arm IT instruction in the unwinder

[Tamas Berghammer via lldb-commits]

tberghammer added a comment.

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?


http://reviews.llvm.org/D16814