[llvm-commits] COFF patch

[Cameron Esfahani]

On Jul 31, 2010, at 1:02 AM, Michael Spencer wrote:

> On Thu, Jul 29, 2010 at 7:56 PM, Cameron Esfahani <dirty at apple.com> wrote:
>> My first patch, so please be gentle...
>> 
>> This fixes a problem I was seeing with the COFF back end.  All of the string data was getting put into one data fragment, and so none of the fixup references were correct.
> 
> Can you post a test case so I can take a look? I'm not quite sure what
> the problem is. But I know this isn't the best way to fix the problem.
> 

Here's a reproducible case: http://pastebin.com/NCActA1d

Build it with: clang -ccc-host-triple i686-pc-win32 -c -integrated-as -o HelloWorld.o HelloWorld.c.

If you dump HelloWorld.o, you'll see that the symbol table entries for L_.str2, L_.str1 and L_.str have values of 0.

Enclosed is, what I believe to be, a better fix for this problem.  Instead of allocating a new data fragment for each new label, just make sure to account for the symbol's offset within the fragment when creating the symbol table value.

This patch is based on revision 110062.

I also noticed that the comment for Code in MCInstFragment was incorrect.  I attempted to fix it, but it's probably horribly wrong...

>> I also made sure the TimeDateStamp in the COFF header had a correct value.
> 
> This is right.
> 
>> I also fixed a spot in MCFragment where Offset wasn't being properly initialized.
>> 
>> This patch was generated against revision 109821.
> 
> Also, was this change required to get it to work? The SymbolTableIndex
> should be set down on line 674.
> 
> @@ -578,12 +580,13 @@ void WinCOFFObjectWriter::RecordRelocation(const
> MCAssembler &Asm,
>   FixedValue = Target.getConstant();
> 
>   COFFRelocation Reloc;
> 
>   Reloc.Data.VirtualAddress = Layout.getFragmentOffset(Fragment);
>   Reloc.Symb = coff_symbol;
> +  Reloc.Data.SymbolTableIndex = 0;
> 
>   Reloc.Data.VirtualAddress += Fixup.getOffset();
> 
>   switch (Fixup.getKind()) {
>   case FirstTargetFixupKind: // reloc_pcrel_4byte
>     Reloc.Data.Type = COFF::IMAGE_REL_I386_REL32;
> 
> - Michael Spencer


The Reloc.Data.SymbolTableIndex = 0 line was required because of this warning:

/usr/include/c++/4.2.1/ext/new_allocator.h: In member function ‘virtual void<unnamed>::WinCOFFObjectWriter::RecordRelocation(const llvm::MCAssembler&, const llvm::MCAsmLayout&, const llvm::MCFragment*, const llvm::MCFixup&, llvm::MCValue, uint64_t&)’:
/usr/include/c++/4.2.1/ext/new_allocator.h:107: warning: ‘Reloc.<unnamed>::COFFRelocation::Data.llvm::COFF::relocation::SymbolTableIndex’ is used uninitialized in this function
WinCOFFObjectWriter.cpp:582: note: ‘Reloc.<unnamed>::COFFRelocation::Data.llvm::COFF::relocation::SymbolTableIndex’ was declared here

-------------- next part --------------
A non-text attachment was scrubbed...
Name: COFFfragment2.patch
Type: application/octet-stream
Size: 2529 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100802/2347c510/attachment.obj>
-------------- next part --------------

Cameron Esfahani
dirty at apple.com

"You only live once, and the way I live, once is enough"

Frank Sinatra