Implement the unwind opcode assembler for .save, .vsave, .setfp, .pad directives.
Details
Diff Detail
Event Timeline
Hi all,
I've tested this and run some C++ benchmark and testsuite before. They work
well on ARM obj emission.
I think it achieves an important milestone for ARM EHABI after this patch.
Could you have a review on it?
Thanks!
It seems I again forgot to submit the comments :(
lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp | ||
---|---|---|
44 | Make sure to include some informative message here. E.g. assert(Index < 16 && "Invalid personality index") | |
377 | Again, please provide here some message | |
429 | What if we'll use streamer to emit object code out of assembly? Shouldn't this assert be a valid error message? Or this will be handled in asmparser? | |
430 | Please be consistent. No need of () around text. |
lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp | ||
---|---|---|
44 | done | |
377 | done. | |
429 | AFAIK, the AsmParser for ARM has not supported these directives yet. The ARM backend is the only user of these functions. Besides, I think it will be better to use assertion at the moment, because some other code are using assertions as well. Maybe we can change many of these assertions to if (cond) report_fatal_error("...") after implementing the AsmParser for these directives in the future. | |
430 | done |
In this version, there are 2 changes compared with the second patch set:
(1) Refine the personality index related code. Now, we encapsulate the personality index in the unwind opcode assembler. The assembler will decide the personality index for us if no .personality or .cantunwind directives are found.
(2) Change several constant literals to named constants.
lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp | ||
---|---|---|
44 | According to EHABI only indices 0, 1 and 2 are allowed. Why 16? | |
337–338 | We should do this regardless of index. The contents declares how we should emit the stuff. Are we going always use _pr1 function? | |
355–357 | The condition seems to be bogus | |
lib/Target/ARM/MCTargetDesc/ARMUnwindOp.h | ||
119 | Add something like max_personality_index and use to to check whether the index is bogus |
lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp | ||
---|---|---|
44 | According to EHABI p.15 - p.16 (top of p.16), it is possible to have 16 personality. Although, EHABI only defines 3 personality routines at the moment. I found that some of libc defines __aeabi_unwind_cpp_pr3 as well. | |
337–338 | For aeabi_unwind_cpp_pr1, we have to emit the opcode in ".extab" section. For aeabi_unwind_cpp_pr0, we have to emit the code in ".exidx" section. For example, the ".exidx" section looks like: [code offset (4 bytes)] [unwind opcodes (4 bytes)] (aeabi_unwind_cpp_pr0) | |
355–357 | In fact, this is equivalent to "UnwindAsmOp.getPersonalityIndex() != INVALID_PERSONALTIY_INDEX". I will change this in next patch set. | |
lib/Target/ARM/MCTargetDesc/ARMUnwindOp.h | ||
119 | OK. But I think I will define it as "MAX_PERSONALITY_INDEX = 0x10u". |
lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp | ||
---|---|---|
44 | It's their problem :). We should not allow stuff we do not support. So, right now 2 is the maximum index. |
Compared with last patch set:
- Rename INVALID_PERSONALITY_INDEX to MAX_PERSONALITY_INDEX.
- Change the assertion to (PI < MAX_PERSONALITY_INDX).
- Change the (!Personality && !CantUnwind) to (PersonalityIndex < MAX_PERSONALITY_INDEX)
- Rename AEABI_CPP_UNWIND_PRx to AEABI_UNWIND_CPP_PRx, so that they can be consistent with the function name.
Several changes since last change:
- Add more comments regarding to the __aeabi_unwind_cpp_pr0 and __aeabi_unwind_cpp_pr1 unwind opcodes generation.
- Emit the unwind opcodes in ".ARM.extab" when __aeabi_unwind_cpp_pr2 is used.
- Rename MAX_PERSONALITY_INDEX to NUM_PERSONALITY_INDEX.
The code is in good shape as it seems to me. However, given that currently there is a claim that there is a test which does not pass - let's wait whether the test will be available. So, the fixed variant can be committed.
lib/Target/ARM/MCTargetDesc/ARMUnwindOpAsm.cpp | ||
---|---|---|
29 | Will it be possible to change these bitmasks to something readable? | |
113 | Missed encoding for opcode? | |
lib/Target/ARM/MCTargetDesc/ARMUnwindOpAsm.h | ||
33 | Do we create so many UnwindOpcodeAssembler's, so we should care about size? Probably not. However, the speed might be sacrificed due to bitfield accesses. |
Change to address comments.
- Change bit field to unsigned (or bool)
- Replace hardcoded opcode numbers with meaningful names (which was missed in previous patch)
- Update testcase we are switching to llvm-readobj these days.
- Refine one byte opcode encoding algorithm (to make the register looks more explicit, and add some comments)
lib/Target/ARM/MCTargetDesc/ARMUnwindOpAsm.cpp | ||
---|---|---|
29 | Hoping the refined version in the new patch looks better now. Please let me know if you have better suggestion. | |
113 | Sorry for missing it. I replaced several of them in the next patch. Thanks. | |
lib/Target/ARM/MCTargetDesc/ARMUnwindOpAsm.h | ||
33 | You are right. There will only be one UnwindOpcodeAssembler per ARMELFStreamer. I'm changing this in the new patch. |
Make sure to include some informative message here. E.g. assert(Index < 16 && "Invalid personality index")