This is an archive of the discontinued LLVM Phabricator instance.

Fix ARM zero-cost exception table (part 8)
ClosedPublic

Authored by logan on Feb 15 2013, 7:36 AM.

Details

Summary

Implement the unwind opcode assembler for .save, .vsave, .setfp, .pad directives.

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!

asl added a comment.Mar 6 2013, 1:56 PM

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.

logan updated this revision to Unknown Object (????).Mar 8 2013, 4:35 AM

This patch adds several description to the assertions.

logan added inline comments.Mar 8 2013, 4:43 AM
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

logan updated this revision to Unknown Object (????).Mar 9 2013, 11:57 PM

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.

asl added inline comments.Mar 10 2013, 11:32 AM
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

logan added inline comments.Mar 11 2013, 2:31 AM
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)
[code offset (4 bytes)] [exception table offset (4 bytes)] (
aeabi_unwind_cpp_pr1)
[code offset (4 bytes)] [exception table offset (4 bytes)] (__gxx_personality_v0)

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".

asl added inline comments.Mar 11 2013, 2:33 AM
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.

logan updated this revision to Unknown Object (????).Mar 11 2013, 8:41 AM

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.
logan updated this revision to Unknown Object (????).Mar 15 2013, 10:38 PM

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.
asl added a comment.Apr 15 2013, 1:35 AM

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
28

Will it be possible to change these bitmasks to something readable?

112

Missed encoding for opcode?

lib/Target/ARM/MCTargetDesc/ARMUnwindOpAsm.h
32

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.

logan updated this revision to Unknown Object (????).Apr 15 2013, 9:55 AM

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)
logan added inline comments.Apr 15 2013, 9:59 AM
lib/Target/ARM/MCTargetDesc/ARMUnwindOpAsm.cpp
28

Hoping the refined version in the new patch looks better now. Please let me know if you have better suggestion.

112

Sorry for missing it. I replaced several of them in the next patch. Thanks.

lib/Target/ARM/MCTargetDesc/ARMUnwindOpAsm.h
32

You are right. There will only be one UnwindOpcodeAssembler per ARMELFStreamer. I'm changing this in the new patch.

logan updated this revision to Unknown Object (????).Apr 15 2013, 11:29 AM

It seems that I missed the bit field change in previous diff.

asl accepted this revision.Apr 15 2013, 12:27 PM

LGTM. Thanks for working on this!

logan closed this revision.Apr 16 2013, 10:05 AM

Thanks for your review. Committed as r179591.