[llvm] r196432 - Fix a bug in darwin's 32-bit X86 handling of evaluating fixups.

[Kevin Enderby]

Hi Rafael.

I would like to use llvm-objdump to disassemble for the test case but given the nature of the bug, that the offset to the item to be relocated needs to be more than 24 bits to trigger the bug, it takes a long time to do the disassembly:

$ time /Volumes/SandBox/build-llvm/Debug+Asserts/bin/llvm-objdump -disassemble /tmp/x.o | grep movl 
 1020f55:	c7 05 64 75 53 01 00 00 00 00                	movl	$0, 22246756

real	0m19.777s
user	0m19.555s
sys	0m0.036s

If llvm-objdump had an option like otool(1)'s  -p one could simply do:

$ time otool -tv -p bug /tmp/x.o 
/tmp/x.o:
(__TEXT,__text) section
bug:
01020f55	movl	$0x0, 0x1537564

real	0m0.690s
user	0m0.004s
sys	0m0.014s

I talked to Jim Grosbach at lunch and he said it was a non-starter to have a test case that takes a long time to run.  So it took me a while to come up with something that was quick.  And I did add a fair bit of comments to the test case to try to explain what is happening.  I agree it is not the best and I too would like to have matched it up with disassembly and not the output of macho-dump.

My thoughts,
Kev

P.S. This bug was a real pain to find given how large the test case was that I started with :)

On Dec 5, 2013, at 8:42 AM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

> Can you use llvm-objdump to disassemble? That should make the test a
> bit more readable.
> 
> On 4 December 2013 18:36, Kevin Enderby <enderby at apple.com> wrote:
>> Author: enderby
>> Date: Wed Dec  4 17:36:24 2013
>> New Revision: 196432
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=196432&view=rev
>> Log:
>> Fix a bug in darwin's 32-bit X86 handling of evaluating fixups.
>> 
>> Where it would use a scattered relocation entry but falls back to a
>> normal relocation entry because the FixupOffset is more than 24-bits.
>> 
>> The bug is in the X86MachObjectWriter::RecordScatteredRelocation() where
>> it changes reference parameter FixedValue but then returns false to indicate
>> it did not create a scattered relocation entry.  The fix is simply to save the
>> original value of the parameter FixedValue at the start of the method and
>> restore it if we are returning false in that case.
>> 
>> rdar://15526046
>> 
>> Added:
>>    llvm/trunk/test/MC/MachO/x86_32-scattered-reloc-fallback.s
>> Modified:
>>    llvm/trunk/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp
>> 
>> Modified: llvm/trunk/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp?rev=196432&r1=196431&r2=196432&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp (original)
>> +++ llvm/trunk/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp Wed Dec  4 17:36:24 2013
>> @@ -362,6 +362,7 @@ bool X86MachObjectWriter::RecordScattere
>>                                                     MCValue Target,
>>                                                     unsigned Log2Size,
>>                                                     uint64_t &FixedValue) {
>> +  uint64_t OriginalFixedValue = FixedValue;
>>   uint32_t FixupOffset = Layout.getFragmentOffset(Fragment)+Fixup.getOffset();
>>   unsigned IsPCRel = Writer->isFixupKindPCRel(Asm, Fixup.getKind());
>>   unsigned Type = MachO::GENERIC_RELOC_VANILLA;
>> @@ -431,8 +432,10 @@ bool X86MachObjectWriter::RecordScattere
>>     // symbol, things can go badly.
>>     //
>>     // Required for 'as' compatibility.
>> -    if (FixupOffset > 0xffffff)
>> +    if (FixupOffset > 0xffffff) {
>> +      FixedValue = OriginalFixedValue;
>>       return false;
>> +    }
>>   }
>> 
>>   MachO::any_relocation_info MRE;
>> 
>> Added: llvm/trunk/test/MC/MachO/x86_32-scattered-reloc-fallback.s
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/MachO/x86_32-scattered-reloc-fallback.s?rev=196432&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/MC/MachO/x86_32-scattered-reloc-fallback.s (added)
>> +++ llvm/trunk/test/MC/MachO/x86_32-scattered-reloc-fallback.s Wed Dec  4 17:36:24 2013
>> @@ -0,0 +1,27 @@
>> +// RUN: llvm-mc -triple i386-apple-darwin9 %s -filetype=obj -o - | macho-dump --dump-section-data | FileCheck %s
>> +
>> +// rdar://15526046
>> +
>> +.text
>> +.globl _main
>> +_main:
>> +       .space 0x01020f55, 0x90
>> +bug:
>> +       movl  $0, _key64b_9+4
>> +.section __TEXT, __padding
>> +       .space 0x515b91, 0
>> +.data
>> +       .space 0xa70, 0
>> +.globl _key64b_9
>> +_key64b_9:
>> +       .long 1
>> +       .long 2
>> +
>> +// The movl instruction above should produce this encoding where the address
>> +// of _key64b_9 is at 0x01537560.  This is testing falling back from using a
>> +// scattered relocation to a normal relocation because the offset from the
>> +// start of the section is more than 24-bits.  But need to get the item to
>> +// be relocated, in this case _key64b_9+4, value correct in the instruction.
>> +// 01020f55    c7056475530100000000    movl    $0x0, 0x1537564
>> +
>> +// CHECK:   90c70564 75530100 000000')
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits