Bug 26556 - relax_segment calculates incorrect target frag when OCTETS_PER_BYTE > 1
Summary: relax_segment calculates incorrect target frag when OCTETS_PER_BYTE > 1
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.34
: P2 minor
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-31 01:02 UTC by Tucker
Modified: 2020-09-18 12:30 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Possible patch (390 bytes, patch)
2020-08-31 01:02 UTC, Tucker
Details | Diff
Minimal example showcasing problem (144 bytes, text/plain)
2020-09-16 20:32 UTC, Tucker
Details
Patch the offset in bss_alloc instead (271 bytes, patch)
2020-09-17 22:26 UTC, Tucker
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tucker 2020-08-31 01:02:28 UTC
Created attachment 12810 [details]
Possible patch

The case statement handling rs_org in write.c around line 2937 does not appear to calculate the correct target fragment address when OCTETS_PER_BYTE > 1

It only applies OCTETS_PER_BYTE scaling to the symbol value, but not the offset.

The end result is symbols in .bss with overlapping addresses.

e.g. from readelf - array_ has a size of 4, but var_ is located at address 2

   Num:    Value  Size Type    Bind   Vis      Ndx Name
     5: 00000000     4 OBJECT  LOCAL  DEFAULT    4 array_
     6: 00000002     1 OBJECT  LOCAL  DEFAULT    4 var_


The attached patch is a possible fix, but II am unsure if this is the correct place to address the issue or if there is another root cause.

Using the above patch the symbol table now looks like so:

   Num:    Value  Size Type    Bind   Vis      Ndx Name
     5: 00000000     4 OBJECT  LOCAL  DEFAULT    4 array_
     6: 00000004     1 OBJECT  LOCAL  DEFAULT    4 var_
Comment 1 Nick Clifton 2020-09-16 12:25:22 UTC
Hi Tucker,

  Would you mind uploading the test case that you are using to examine this problem please ?

Cheers
  Nick
Comment 2 Tucker 2020-09-16 20:32:26 UTC
Created attachment 12846 [details]
Minimal example showcasing problem
Comment 3 Tucker 2020-09-16 20:35:12 UTC
Sure thing Nick. I've attached some assembly that showcases the issue.

To reproduce, build a target with OCTETS_PER_BYTE_POWER set to 1. I choose MSP430 as a victim.

Normal MSP430:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     5: 00000000     4 OBJECT  LOCAL  DEFAULT    3 array_
     6: 00000004     1 OBJECT  LOCAL  DEFAULT    3 var_


OCTETS_PER_BYTE_POWER = 1 MSP430:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     5: 00000000     4 OBJECT  LOCAL  DEFAULT    3 array_
     6: 00000002     1 OBJECT  LOCAL  DEFAULT    3 var_
Comment 4 Nick Clifton 2020-09-17 16:16:41 UTC
Hi Tucker,

  Well this patch breaks some of the gas tests for targets currently
  use OCTETS_PER_BYTE > 1.  (For example try tic54x-coff and look at
  the gas/org-3 test).

  In fact I do not see any ELF targets that use OCTETS_PER_BYTE > 1,
  so I am wondering where this issue is coming from.  Do you have a
  new target that is using this feature, or a COFF/PE based target
  that is experiencing problems ?

  Anyway, my current feeling is that write.c:relax_segment() is 
  probably not the right place to fix this problem.  My gut feeling
  is that the fr_offset field in the frag that is being used for
  the common definitions needs to be octet adjusted at some earlier
  point.  Precisely which point I do not know.

Cheers
  Nick
Comment 5 Tucker 2020-09-17 18:57:16 UTC
Hi Nick,

Thanks for the information regarding tic54x-coff and gas/org-3 test. I will re-investigate the issue with this target in mind.

Regarding the target: Yes, this is a new ELF target with 16 bit bytes I've been experimenting with. The legacy tool-chain is horrific, and I've been trying to get better tools functioning on it as a side project.

-Tucker
Comment 6 Tucker 2020-09-17 22:26:28 UTC
Created attachment 12848 [details]
Patch the offset in bss_alloc instead

Nick,

Traced the source of fr_offset back to bss_alloc. Applying the octet adjustment here seems to fix the issues as well as keep the org test cases happy.

Just food for thought.
Comment 7 cvs-commit@gcc.gnu.org 2020-09-18 12:29:50 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=5947daaf7545b2887abb977af6700b4cc9a807be

commit 5947daaf7545b2887abb977af6700b4cc9a807be
Author: Nick Clifton <nickc@redhat.com>
Date:   Fri Sep 18 13:28:46 2020 +0100

    Ensure that space allocated by assembler directives converts from an octet count to a byte count.
    
            PR 26556
            * read.c (bss_alloc): Convert size parameter from octets to
            bytes.
Comment 8 Nick Clifton 2020-09-18 12:30:48 UTC
Works for me.  Patch committed.