[PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Christian Eggers]

forgot the subject in my first mail....

On Wed, 18 Dec 2019 12:42:57 +1030, Alan Modra wrote:
> Overall this is looking good, and I think your decision to make the
> internal units for p_vaddr, p_paddr and sh_addr octets has worked out
> well.

Sorry for responding late. I do the binutils SDMA port in my leisure
time. In the last two months I had to focus on my primary work and on my
family. I hope that upstreaming of my SDMA work can be finished soon.

> But it looks like you have missed a few places that need an octet per
> byte factor...
In order to ease review for you, I did only slight changes on the v2
patch and added the missing parts in a follow-up patch.

> ...eg. elf.c search for filehdr_vaddr.  Symbol values for
> an address are in bytes, at least internally.
added in the follow-up.

> There's another one at
> around line 7190 of elf.c where we have a comparison of an expression
> involving p_vaddr and a section vma.
also added in the follow-up.

> That one raises the question of
> what to do with alignment.  Presumably, section->alignment_power stays
> in bytes since it is most often used with section->vma.
I added a comment on the struct member in the follow-up patch.

> So that's
> another thing that needs converting if used to align p_vaddr/p_paddr.
In the follow-up, I also checked all users of section->alignment_power.

> Also elfcode.h bfd_from_remote_memory uses p_vaddr without converting
> to bytes.
done in the follow-up.

> Also search for phdr_lma in elf.c.
done in the follow-up.

> IS_CONTAINED_BY_LMA needs a fix too,
changed in v3 of the first patch

> and this in elf.c "map->p_paddr = matching_lma->lma"
done in the follow-up

> and the alignment a little later.
unsure, maybe I didn't find

> You also need to correct code in bfd_elf_final_link using sh_addr when
> setting up DT_REL/DT_RELA.  There's a comparison between sh_addr and
> vma.
done in the follow-up

> The following in elf32-rx.c where octets per byte is always one (and
> thus the file needs no change) is bogus, please remove, and the change
> to elf64-ia64-vms.c too:
removed in v3 of the first patch

regards
Christian

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Christian Eggers]

Dear Alan,

On Wed, 18 Dec 2019 12:42:57 +1030, Alan Modra wrote:
> I'm also wondering about relocations, symbols and dynamic tags with
> address values.
I'm unsure whether I can answer this questions correctly because currently "it
simply works for me". But I will try...

> Does r_offset specify bytes or octets in the ELF file?
As far I can see, struct elf_internal_rela::r_offset is set from
struct reloc_cache_entry::address [1].

from my gas/config/tc-sdma.c:
arelent *
sdma_cgen_tc_gen_reloc (asection *section, fixS *fixP)
{
  arelent *reloc = gas_cgen_tc_gen_reloc(section, fixP);
  if (! (section->flags & SEC_ELF_OCTETS))
    {
      reloc->address >>= OCTETS_PER_BYTE_POWER;
    }

  return reloc;
}

So it depends on whether SEC_ELF_OCTETS is set for the section where the
relocation is installed. Particularly relocations offsets inside .debug_line
are often not aligned to target bytes, see we need octets here.

> If octets, then do we convert in elf_swap_reloca_in/out making
> internal r_offset in bytes which should be most convenient since it is
> often used with section vma?
I think that no further conversion should be done here.

> How about r_addend
Usually in *bytes*, but for sections like .debug_str, *octets* are required.
So it depends on whether SEC_ELF_OCTETS is set for the section of the symbol.

> and st_value?
debugging through gas showed that struct elf_internal_sym::st_value is set
from struct bfd_symbol::value [2] which in turn is set from struct
expressionS::X_add_number [3].

For sections like .text, all these values are in *bytes*. For sections which
have SEC_ELF_OCTETS set (like .debug_info), there exists only the section
symbols which have a value of 0. In absence of of "real" symbols inside these
sections, I cannot say whether st_value can also be in octets.

> Anyway, those can be left for a later patch if necessary.
I hope this could clarify you questions. As I already said, from the point of
the SDMA, everything works fine as it is.

regards
Christian

[1] bfd/elfcode.h:978
[2] bfd/elf.c:8139/8192
[3] gas/write.c:2311/2410

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Christian Eggers]

Dear Alan,

some hours after my previous message I became doubtful whether the current
state is correct. As SDMA will be the first ELF target with opb>1, I see a
chance for getting things right before more targets appear and everything is
cemented.

Am Sonntag, 16. Februar 2020, 09:22:22 CET schrieb Christian Eggers:
> On Wed, 18 Dec 2019 12:42:57 +1030, Alan Modra wrote:
> > I'm also wondering about relocations, symbols and dynamic tags with
> > address values.
>
> I'm unsure whether I can answer this questions correctly because currently
> "it simply works for me". But I will try...
>
> > Does r_offset specify bytes or octets in the ELF file?
>
> > How about r_addend
>
> > and st_value?

As SEC_ELF_OCTETS is only available for BFD programs, mixing bytes and octets
in the external representation (ELF) maybe a poor decision.

Initially everything was in bytes, but this wasn't suitable for DWARF
information as these are only representable in octets. In order to get around
this, I introduced SEC_ELF_OCTETS, which helped to keep the internal
representation (VMA/LMA) in bytes.

But for the ELF file, every program handling it must know which information is
in octets or in bytes. For luck, the sizes are already in octets, otherwise I
had already trouble with external software. But probably everything should be
stored in octets, at least this should resolve the trouble I already have with
my external debugger. This debugger has only one symbol space for ARM and
SDMA. Having symbols (and debug line info) in octets should make nasty
workarounds needless.

What is your opinion?

Regards
Christian

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Alan Modra]

On Sun, Feb 16, 2020 at 08:37:43PM +0100, Christian Eggers wrote:
> But for the ELF file, every program handling it must know which information is
> in octets or in bytes. For luck, the sizes are already in octets, otherwise I
> had already trouble with external software. But probably everything should be
> stored in octets, at least this should resolve the trouble I already have with
> my external debugger. This debugger has only one symbol space for ARM and
> SDMA. Having symbols (and debug line info) in octets should make nasty
> workarounds needless.
> 
> What is your opinion?

Yes, that is what I'd be inclined to so.  In fact, if faced with
designing an ABI for your NXP SDMA processor from scratch I'd avoid
the octets_per_byte conversion entirely by using octets from assembly
onwards.  Addresses would always be multiples of two, and you'd design
relocations to suit.  Users of course would need to write ". + 2"
rather than ". + 1" and similarly in other expressions involving
addresses.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Christian Eggers]

Am Donnerstag, 20. Februar 2020, 02:29:53 CET schrieb Alan Modra:
> On Sun, Feb 16, 2020 at 08:37:43PM +0100, Christian Eggers wrote:
> > But for the ELF file, every program handling it must know which
> > information is in octets or in bytes. For luck, the sizes are already in
> > octets, otherwise I had already trouble with external software. But
> > probably everything should be stored in octets, at least this should
> > resolve the trouble I already have with my external debugger. This
> > debugger has only one symbol space for ARM and SDMA. Having symbols (and
> > debug line info) in octets should make nasty workarounds needless.
> >
> > What is your opinion?
>
> Yes, that is what I'd be inclined to so.  In fact, if faced with
> designing an ABI for your NXP SDMA processor from scratch I'd avoid
> the octets_per_byte conversion entirely by using octets from assembly
> onwards.  Addresses would always be multiples of two, and you'd design
> relocations to suit.  Users of course would need to write ". + 2"
> rather than ". + 1" and similarly in other expressions involving
> addresses.

When I started SDMA development, after some days I stumbled over
"octets_per_byte" and thought that this is the correct way working with
processors which address 16 bit per program counter value. If I had known how
much problems this will cause, maybe I had never tried...

From the git history it looks like you already took a lot of effort to get
octets_per_byte working. If also found PR19713 (and the following), were Dan
Gissel worked on an ELF port for octets_per_byte=4 (but I don't know any
details). As most of SDMA (including the test suites) currently works fine
with the current OPB setting, I would prefer to keep it that way.

For the ELF representation, I currently study whether using octets everywhere
would be an improvement. Here is what I found so far:

Elf_Internal_Rela::r_offset
----------------------------
Currently, arelent::address is produced by tc_gen_reloc
[sdma_gen_tc_gen_reloc], either as octets or bytes, depending on
SEC_ELF_OCTETS. This is copied to Elf_Internal_Rela::r_offset in
elf_write_relocs().

The inverse happens in elf_slurp_reloc_table_from_section() where
Elf_Internal_Rela::r_offset is copied to arelent::address. The main user is
bfd_perform_relocation(), where address is multiplied with
bfd_octets_per_byte (..., input_section).

Possible change: Keep arelent::address as it is, change
Elf_Internal_Rela::r_offset to octets. Change the data type of r_offset to a
new type (e.g. bfd_octet) in order to annotate the unit.

Users of Elf_Internal_Rela::r_offset:
- _bfd_elf_discard_section_eh_frame(): /* FIXME: octets_per_byte.  */
- _bfd_elf_parse_eh_frame()
  + ENSURE_NO_RELOCS(): /* FIXME: octets_per_byte.  */
  + SKIP_RELOCS() : /* FIXME: octets_per_byte.  */
  + GET_RELOC(): /* FIXME: octets_per_byte.  */
- bfd_elf_perform_complex_relocation(): Calls bfd_octets_per_byte(..., sec)
- bfd_elf_reloc_symbol_deleted_p(): called by
  + _bfd_discard_section_stabs(): unsure, may need conversion to octets?
  + _bfd_elf_discard_section_eh_frame(): /* FIXME: octets_per_byte.  */
- elf_swap_reloc(a)_in/out(): No change
- elf_write_relocs(): Change: Convert arelent::address to octets.
- elf_gc_smash_unused_vtentry_relocs(): Currently buggy, hend is composed of
bytes and octets.
- elf_link_input_bfd():
  + Needs four conversions
  + Called function _bfd_elf_section_offset() should return octets
- elf_link_read_relocs_from_section(): Only printfs
- elf_link_sort_cmp1(): No change
- elf_link_sort_cmp2(): No change
- elf_link_sort_relocs(): Copy to elf_link_sort_rela::offset, should also be
converted (trivial).
- elf_reloc_link_order(): Needs one conversion
- elf_slurp_reloc_table_from_section(): Needs two conversions
- mark_entry():  /* FIXME: octets_per_byte.  */
Other users are in readelf.c which would probably be more happy with octets
(at least if debug_info also is).


Conclusion: "Changing" Elf_Internal_Rela::r_offset to octets would probably
remove more FIXMEs than it creates.

Bonus question: What about Elf_Internal_Rela::r_offset?
For me it looks like this should be kept as it is.

I will investigate the other ELF variables in a further email (after the kids
are finished shouting through the house).

Regards
Christian

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Christian Eggers]

> > How about r_addend
>
> Usually in *bytes*, but for sections like .debug_str, *octets* are required.
> So it depends on whether SEC_ELF_OCTETS is set for the section of the
> symbol.

In order to generate a relocation in GAS, at first a fixup is created:

gas_cgen_finish_insn()
+--gas_cgen_record_fixup_exp()
   +--fix_new_exp()
      +--fix_new_internal()   <-- allocation of fixS object

In case the relocation is PC relative, MD_PCREL_FROM_SECTION(fix, section) is called later:

write_object_file()
+--bfd_map_over_sections (..., fix_segment, ...);
   +--fix_segment()
      +--fixup_segment()
         +--md_pcrel_from_section()
            ...
            return (fixP->fx_where + fixP->fx_frag->fr_address) >> OCTETS_PER_BYTE_POWER

While fixP->fx_where is in octets, the result has to be converted to bytes. It
is used within fixup_segment for calculations with other byte values:

...
add_number = fixP->fx_offset;
...
add_number += S_GET_VALUE (fixP->fx_addsy);
fixP->fx_offset = add_number;
if (fixP->fx_pcrel)
  add_number -= MD_PCREL_FROM_SECTION (fixP, this_segment);
...

All fix ups, which could not be fixed up, are converted to relocations in
gas_cgen_tc_gen_reloc(). Here fixS::fx_offset/fx_addnumber is copied to
arelent::addend:

write_object_file()
+--bfd_map_over_sections()
   +--write_relocs()
      +--tc_gen_reloc()
         +--gas_cgen_tc_gen_reloc()
            ...
            reloc->addend = fixP->fx_addnumber;
            ...

All users of arelent::addend clearly deal with bytes, not octets.

Conversion between arelent::addend and elf_internal_rela::r_addend happens in
elf_slurp_reloc_table_from_section() and elf_write_relocs().

elf_internal_rela::r_addend has the following users:
- _bfd_elf_rela_local_sym():
  + rel->r_addend = _bfd_merged_section_offset(.., sym->st_value + rel->r_addend)
    * Have a look at st_value later
    * _bfd_merged_section_offset() deals with octets, not bytes
  + Two additional conversion required.
- bfd_elf_perform_complex_relocation()
  + decode_complex_addend(..., rel->r_addend)
    * r_addend has special meaning for complex relocations and must not be
      altered in this case
- elf_swap_reloc(a)_in/out(): No change
- elf_write_relocs(): Change: Convert arelent::address to octets (depending on SEC_ELF_OCTETS)
- elf_gc_smash_unused_vtentry_relocs(): No change
- elf_link_input_bfd: Four additional conversion required
- elf_reloc_link_order(): Copy from bfd_link_order_reloc::addend. That should be kept in bytes.
- elf_slurp_reloc_table_from_section(): Convert from octets to bytes (depending on SEC_ELF_OCTETS)

All other users are in readelf, which expects octets in all cases.

Conclusion: "Changing" Elf_Internal_Rela::r_addend to octets looks
straightforward as for r_offset.

At next, I will have a look for st_value and debug info.

Regards
Christian

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Christian Eggers]

> > and st_value?
>
> debugging through gas showed that struct elf_internal_sym::st_value is set
> from struct bfd_symbol::value [2] which in turn is set from struct
> expressionS::X_add_number [3].
>
> For sections like .text, all these values are in *bytes*. For sections which
> have SEC_ELF_OCTETS set (like .debug_info), there exists only the section
> symbols which have a value of 0. In absence of of "real" symbols inside
> these
> sections, I cannot say whether st_value can also be in octets.
> ...
> [2] bfd/elf.c:8139/8192
> [3] gas/write.c:2311/2410

elf_internal_sym::st_value has the following users:
- _bfd_elf_rel_local_sym():
  + Parameter "added" is sometimes octets, sometimes bytes.
    * Only used for architectures where this doesn't matter
  + _bfd_merged_section_offset() deals with octets, not bytes
  + result should be kept in bytes
- _bfd_elf_rela_local_sym(): Needs one conversion
- adjust_eh_frame_local_symbols():
  + delta = offset_adjust (sym->st_value, sec)
    * offset_adjust() uses octets/bytes inconsistently, needs fixes
      - eh_cie_fde::offset should be kept in octets
    * offset_adjust() should better return bytes
- asymbol_from_plugin_symbol(): No change?
- bfd_elf_final_link(): Needs 4 conversions.
- bfd_elf_print_symbol(): Only printf, may be kept as it is
- elf_slurp_symbol_table(): Copies to bfd_symbol::value, needs conversion
- elf_swap_symbol_in/out(): No change
- elf_common_parse():
  ...
  align = parse_align (is_local);
  ...
  S_SET_ALIGN (symbolP, align);
  +-- st_value = align ???   <--- why? But needs conversion
- elf_link_add_object_symbols(): More complicated!
  + bfd_vma value = isym->st_value  <-- octets
    ...
    value = isym->st_size;          <-- octets
    ...
    value -= sec->vma;              <-- bytes, needs conversion to octets
    ...
    (*bed->elf_add_symbol_hook) (..., &value))  <-- ???
    ...
    _bfd_elf_merge_symbol (..., &value, ...)    <-- octets
    ...
    _bfd_generic_link_add_one_symbol(..., value, ...)  <-- mixed ???
    ...
    _bfd_elf_add_default_symbol (..., value, ...): _bfd_elf_merge_symbol() <-- octets

  + align = bfd_log2 (isym->st_value);          <-- needs conversion to bytes
  + common_align = bfd_log2 (isym->st_value);   <-- needs conversion to bytes
- elf_link_input_bfd(): Needs 7 conversions
- elf_link_output_extsym(): Needs 4 conversions
- elf_output_implib(): Needs 1 conversion
- elf_s_get_align(): S_GET_ALIGN() <-- ???
- elf_s_set_align(): S_SET_ALIGN() <-- ???
- ldelf_ctf_symbols_iter_cb(): Copies to ctf_link_sym_t::st_value <-- no change
- s_mri_common():
  + align = get_absolute_expression ();
    ...
    S_SET_ALIGN (sym, align);   <-- conversion from bytes to octets

- set_symbol_value(): Needs one conversion
- swap_out_syms(): Needs two conversion


Conclusion: Conversion of elf_internal_sym::st_value from bytes to octets looks
possible, but may discover some currently undefined behavior. Simply converting
everything from bytes to octets in elf_swap_symbol_in/out() could be a shortcut
as currently all non-zero symbol values are in bytes. But doing so would be
inconsistent to my proposed change for r_offset and r_addend, so I would try
the "hard" way.

At next I will have a look at DWARF2 debug information.

Regards
Christian

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Alan Modra]

On Sun, Feb 23, 2020 at 10:07:50AM +0100, Christian Eggers wrote:
> Conclusion: Conversion of elf_internal_sym::st_value from bytes to octets looks
> possible, but may discover some currently undefined behavior. Simply converting
> everything from bytes to octets in elf_swap_symbol_in/out() could be a shortcut
> as currently all non-zero symbol values are in bytes. But doing so would be
> inconsistent to my proposed change for r_offset and r_addend, so I would try
> the "hard" way.

Symbols are a real problem, not because of some implementation detail,
but because you don't know the units of st_value.  A symbol defined as
a label on a line of assembly holds an address, but for example a
symbol defined by
  sym = 123
could be a size in octets, an address, or just a plain number.

I think that if you decide that symbol values should always be octets,
then you'll eventually come to the conclusion that addresses in the
assembler need to be octets too.  And then it's just easier to
dispense with octets_per_byte.

So if you're going to keep octets per byte of 2, I wouldn't be trying
to convert any symbol values.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Christian Eggers]

Am Donnerstag, 27. Februar 2020, 09:35:41 CET schrieb Alan Modra:
> Symbols are a real problem, not because of some implementation detail,
> but because you don't know the units of st_value.  A symbol defined as
> a label on a line of assembly holds an address, but for example a
> symbol defined by
>   sym = 123
> could be a size in octets, an address, or just a plain number.
Maybe this is not the main problem (at least for SDMA). All "plain" numbers in
assembler are most likely bytes. If somebody requires octets in future, a new
".octet <sym>" directive might help...

> I think that if you decide that symbol values should always be octets,
> then you'll eventually come to the conclusion that addresses in the
> assembler need to be octets too.
I've made a short try to store st_value and .debug_line statements in octets.
For this experiment I simply multiplied st_value by 2 in the swap_in and
swap_out functions. While this worked quite well (as all "octet" symbols had a
value of 0), making this change for debug information ended in finally giving
up. It started harmless with expressions which became fixes. But then some of
them became relocations where it is difficult/impossible to know which ones
have to be shifted and which ones not...

> And then it's just easier to dispense with octets_per_byte.
Tried to translate the outcome from German into English: Hindsight is easier
than foresight.

When I started with SDMA, I already had a presentiment that support for
octets_per_byte might be incomplete for ELF/DWARF. But I liked the possibility
of having correct addresses in assembler code and in objdump output and I
didn't want to use the obsolete coff/stabs format.

I suspect that using octets_per_byte=1 for SDMA would complicate things for
the user and additionally would set back my SDMA port by several months (maybe
that I will be unable to complete it in this case).

> So if you're going to keep octets per byte of 2, I wouldn't be trying
> to convert any symbol values.
As I don't want to add further complexity to this, I would like to keep
st_value, r_addend and r_offset like it is for now.


If you are ok with my latest patches, I would like send the first version of
the SDMA port. After working countless weekends on my first binutils port,
it's time to finish...

Regards
Christian

Re: [PATCH v3 0/2] Fix several mix up between octets and bytes in ELF program headers

[Christian Eggers]

And now the debug information:

Currently, expressions within the debug sections are resolved either to octets
or to bytes (depending on SEC_ELF_OCTETS of the symbol in the expression).
This happens within GAS, so these expression will not appear as external
symbols nor relocations.

Example 1:
  TC_DWARF2_EMIT_OFFSET (section_symbol (info_seg), sizeof_offset);

resolves to octets as info_seg belongs to the .debug_info section which has
SEC_ELF_OCTETS set

Example 2:
  /* DW_AT_high_pc */
  ...
  exp.X_add_symbol = all_segs->text_end;

resolves to bytes as the .text section has no SEC_ELF_OCTETS set

The proposed change is that expressions / fixes with symbols of a non
SEC_ELF_OCTETS section...
- resolve to octets if the expression appears in a SEC_ELF_OCTETS section
- resolve to bytes if the expression appears inside another section

So the result of a fix must be multiplied with octets_per_byte if the symbol
value is bytes (e.g. !(S_IS_OCTETS (fixP->fx_addsy))) and the section of the
fix has SEC_ELF_OCTETS set. Possibly this could be done in fixup_segment()
when calling md_apply_fix().

As an alternative approach, the conversion from bytes to octets could be
"manually" triggered by setting a new flag inside expressionS / fixS, which
can be checked in fixup_segment().

- out_debug_line():
  + process_entries(): Needs 3 conversions
    + out_set_addr():  No changes
      + emit_expr()
- out_debug_ranges():  No changes
  + emit_expr()
- out_debug_aranges(): No changes
  + emit_expr()
- out_debug_abbrev():  No changes
- out_debug_str():     No changes (already octets)
- out_debug_info():    No changes
  + emit_expr()

Conclusion: In order to change debug information from bytes to octets, it
should be sufficient to perform a conversion for the generated debug line
info. Additionally, fixes applied within SEC_ELF_OCTETS sections may need
conversion to octets if the symbol value resolved to bytes.


If I could get some feedback for the proposed changes to r_offset, r_added,
st_value and debug_info, I would implement the changes soon. I hope that
together with my previous patches, the octets/bytes topic can be closed then.

Regards
Christian