[PATCH] ARM unwind directive parser, assembler, and testcases.

Logan Chien tzuhsiang.chien at gmail.com
Wed Apr 24 04:10:05 PDT 2013



================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:407
@@ -402,8 +406,3 @@
 
-  // Emit Personality
-  assert(Personality && ".personality directive must preceed .handlerdata");
-
-  const MCSymbolRefExpr *PersonalityRef =
-    MCSymbolRefExpr::Create(Personality,
-                            MCSymbolRefExpr::VK_ARM_PREL31,
-                            getContext());
+  // Emit personality
+  if (Personality) {
----------------
Jim Grosbach wrote:
> Is it ever correct to not have a personality function? I would think not, and the assert() was correct.
It is possible to have .handlerdata directive without the .personality, for example, gas can assemble eh-directive-handlerdata.s in the test case without problems.

If the user did not specified the personality, such as __gxx_personality_v0, with the .personality directive, the assembler will pick either __aeabi_unwind_cpp_pr0 or __aeabi_unwind_cpp_pr1 as the personality routine, and the compact model will be used.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:434
@@ -428,1 +433,3 @@
+  if (SPOffset != 0)
+    report_fatal_error("Current implementation assumes .setfp precedes .pad");
 
----------------
Jim Grosbach wrote:
> This should be in the parser as noted above. That said, can you elaborate on why the implementation assumes this? Should it? Basically, I don't understand what this message is trying to say, when it would be encountered, etc..
OK.  I will change this back to assert.

This assertion was written because we need special treatment when the .pad directive comes before .setfp directive which is not implemented yet.  You may get different object file (compared with the one generated by gas) if you are trying to assemble following assembly:

.pad #0x08
sub sp, sp, #0x08
.setfp fp, sp, #0x04
add fp, sp, #0x04


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:328
@@ -326,2 +327,3 @@
 void ARMELFStreamer::EmitFnStart() {
-  assert(FnStart == 0);
+  if (FnStart)
+    report_fatal_error("Previous .fnstart directive is not closed.");
----------------
Jim Grosbach wrote:
> Diagnostics like this really should be part of the asmparser, not the streamer. Here, we don't have any source location information to do anything useful. By this point, an assert() is correct.
> 
> Ditto for the similar changes below.
OK.  I'll come up with a revised patch soon.


http://llvm-reviews.chandlerc.com/D692

BRANCH
  add-arm-unwind-directives-squashed

ARCANIST PROJECT
  llvm



More information about the llvm-commits mailing list