pReloc.updateAddend called in the wrong place

[GoogleCodeExporter]

What version of the product are you using? On what operating system?

Trunk.

Please provide any additional information below.

Right now, pReloc.updateAddend() is called in the wrong place under 
scanRelocations. The problem is that scanRelocations would add/consume entries 
in the .got/.got.plt sections. 

Any relocations that appear in the data sections, especially .ctors/.dtors will 
have the issue that the Addend will get the wrong value and the wrong function 
being called.

This was happening in Hexagon. 

The way this was fixed in our local branch was

a) pReloc.updateAddend() was called only for partialScanRelocation
b) the symbolValue code was changed 

+++ b/lib/Fragment/Relocation.cpp
@@ -92,7 +92,8 @@ Relocation::Address Relocation::symValue() const
 {
   if (m_pSymInfo->type() == ResolveInfo::Section &&
      m_pSymInfo->outSymbol()->hasFragRef()) {
-    return 
m_pSymInfo->outSymbol()->fragRef()->frag()->getParent()->getSection().addr();
+    uint32_t offset = m_pSymInfo->outSymbol()->fragRef()->getOutputOffset();
+    return 
m_pSymInfo->outSymbol()->fragRef()->frag()->getParent()->getSection().addr() + 
offset;
   }
   return m_pSymInfo->outSymbol()->value();

With this all the tests are working fine.

Original issue reported on code.google.com by shanka...@gmail.com on 1 Jun 2013 at 12:15

[GoogleCodeExporter]

Original comment by pete.c...@gmail.com on 5 Sep 2013 at 3:39

[GoogleCodeExporter]

I'm thinking if it's possible to remove Relocation::updateAddend(), and 
attached please find the patch.

Hi Shankar,
Do you still prefer to keep updateAddend() and override 
Relocator::partialScanRelocation() for partial linking? What's the concern if 
we modify the target content directly? (i.e., .start section in 
test/Hexagon/hexagon-relocatable.ll)

Hi Simon,
We found MIPS has similar code in MipsRelocationInfo::sectOff() instead of 
using updateAddend(). Does the change make sense to you? MIPS32 tests (make 
check and ndk testsuite) are still working fine with the change. But we'll have 
different MIPS64 outputs. (Mips/dso/64-so.ts and Mips/exe/64-exe.ts)

Original comment by pete.c...@gmail.com on 27 Sep 2013 at 7:43

Attachments:

• 0001-Relocation-Remove-Relocation-updateAddend.patch

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

The patch is looking good to me. It does not break mips64 and I will update the 
outdated golden files after you commit the patch.

Original comment by si...@atanasyan.com on 27 Sep 2013 at 9:11

[GoogleCodeExporter]

Yes, I would like to use updateAddend for partial linking.

Original comment by shanka...@gmail.com on 27 Sep 2013 at 4:16

[GoogleCodeExporter]

OK! updateAddend is still available for those use RELA.

commit 764ada7f1cbeadd93f1a6d6a78a288e3c011d9fb
Author: Chinyen Chou <petechou@gmail.com>
Date:   Mon Sep 30 09:58:54 2013 +0800

    [Relocation] Refine updateAddend usage.

    Actually we keep input section symbols, so it's possible to get
    the symbol values directly. As for partial linking, we can refer to
    Relocator::partialScanRelocation(), and HexagonRelocator if the
    backend also uses RELA.

Original comment by pete.c...@gmail.com on 30 Sep 2013 at 2:20

[GoogleCodeExporter]

Original comment by pete.c...@gmail.com on 30 Sep 2013 at 2:21

• Changed state: Fixed

[GoogleCodeExporter]

Issue 87 has been merged into this issue.

Original comment by pete.c...@gmail.com on 30 Sep 2013 at 2:23