[PATCH} Review request : Direct object exception handling for Mips

[Jack Carter]

Rafael,

Thank you for looking at this. Attached is my latest patch for this.

Yes, we are sure it is a regression. Our runs of the llvm test-suite have been failing for direct object and after performing a git bisect we narrowed it down to the patch for Revision 177141.

I am not an expert on exception handling, so I depend on what gcc produces and what runs. Making the change in my patch made the pain go away and we match gcc .eh_frame section contents.

On the question of a test case. Thank you for pointing out that I did not include an assembler tests. That is not enough though. Although the assembler uses much of the code that the integrated assembler uses, there are different paths. We need to test for both. I believe we will have to agree to disagree on this one.

I have also added test/MCMips/fde-reloc.s as suggested. It doesn't check the contents of the .eh_frame section, but it did catch a problem with a relocation in 64 bit mode.

I would love to have a dump similar to readobj -wf, but I do not know of one existing in llvm land:

mips64_direct: readelf -wf j.o
Contents of the .eh_frame section:

00000000 00000018 00000000 CIE
  Version:               1
  Augmentation:          "zPLR"
  Code alignment factor: 1
  Data alignment factor: -4
  Return address column: 31
  Augmentation data:     00 00 00 00 00 00 00

  DW_CFA_def_cfa: r29 ofs 0

0000001c 0000001c 00000020 FDE cie=00000000 pc=00000000..0000008c
  Augmentation data:     00 00 00 00

  DW_CFA_advance_loc: 12 to 0000000c
  DW_CFA_def_cfa_offset: 24
  DW_CFA_advance_loc: 8 to 00000014
  DW_CFA_offset: r31 at cfa-4
  DW_CFA_offset: r16 at cfa-8
  DW_CFA_nop
  DW_CFA_nop
  DW_CFA_nop

Regards,

Jack
________________________________________
From: Rafael Espíndola [rafael.espindola at gmail.com]
Sent: Tuesday, March 26, 2013 12:13 PM
To: Jack Carter
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH} Review request : Direct object exception handling for Mips

> Revision 177141 caused a regression in all but
> mips64 little endian. That is because none of the
> other Mips targets had test cases checking the
> contents of the .eh_frame section. This patch fixes
> both the llvm code and the test case to include the
> current 4 flavors.

Are you sure this was a regression? Is mips really the only
architecture using dwarf::DW_EH_PE_absptr? That looks really
suspicious. Is that what the gnu assembler does? If that is the case,
the patch should have a comment on why that is.

Second, this should not use a .ll to .o test. It should use a .s to .o
test. You should probably just update fde-reloc.s to also test mips
(now that I think of it, it probably has to be copied to test/MC/X86
and test/MC/Mips). Note that the test has no assembly instructions,
just cfi directives, so it already works on mips (I just tested that
with "llvm-mc fde-reloc.s -o test.o -filetype=obj -triple
mipsel-unknown-linux").

> Jack

Cheers,
Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug900_v2.patch
Type: text/x-patch
Size: 10622 bytes
Desc: bug900_v2.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130329/e68912c0/attachment.bin>