This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [Patch macho/bfd/gas] .indirect_symbol, take 2.
- From: Tristan Gingold <gingold at adacore dot com>
- To: Iain Sandoe <developer at sandoe-acoustics dot co dot uk>
- Cc: binutils Development <binutils at sourceware dot org>
- Date: Tue, 10 Jan 2012 14:03:18 +0100
- Subject: Re: [Patch macho/bfd/gas] .indirect_symbol, take 2.
- References: <56BB999A-CDA7-4F2C-B58A-B4788E9B7034@sandoe-acoustics.co.uk>
On Jan 9, 2012, at 2:00 PM, Iain Sandoe wrote:
> Here is a different implementation of the .indirect_symbol patch.
>
> In this we keep indirect_symbols in a linked list in the section to which they belong.
I don't think that linked list is the best choice. The overhead is large (at least 2x), and the list structure is useful only for creating it (i.e. for gas). I would prefer to have an array in BFD, and the linked list in GAS.
> As of now, I can't see a reason to keep them in GAS as well.
>
> If this is OK, then a TODO would be to populate the list when a mach-o file is read in -
> - this is nicely symmetrical -
> - and means that sections can be reordered/removed without having to cross-check the dysymtab.
>
> The only thing that is less satisfactory, is that there is no way (AFAICS) to mark a symbol as 'referenced by an indirect' which means if one deletes (or wishes to delete) symbols - then one should cross-check the usage from the indirect tables.
Yes, this is an issue. I need to investigate how to address that.
> tests follow as separate patch (they've all be posted before anyway).
Thanks!
> OK?
See some additional comments.
Tristan.
> Iain
>
> bfd:
>
> * mach-o.c (bfd_mach_o_count_indirect_symbols): New.
> (bfd_mach_o_build_dysymtab_command): Populate indirect table.
> * mach-o.h (bfd_mach_o_section): Add fields for indirect symbol
> lists.
>
> gas:
>
> * config/obj-macho.c (obj_mach_o_set_symbol_qualifier): Switch off
> lazy for extern/private extern.
> (obj_mach_o_indirect_symbol): New.
> (obj_mach_o_placeholder): Remove.
> (mach_o_pseudo_table): Use obj_mach_o_indirect_symbol.
> (obj_macho_frob_label): Take care not to force local symbols into
> the regular table.
> (obj_macho_frob_symbol): Likewise. Ensure undefined and comm syms
> have their fields set.
> (obj_mach_o_frob_section): New.
> * config/obj-macho.h (obj_frob_section): Define.
> (obj_mach_o_frob_section): Declare.
>
> bfd/mach-o.c | 63 +++++++++++++-
> bfd/mach-o.h | 16 ++++
> gas/config/obj-macho.c | 214 +++++++++++++++++++++++++++++++++++++++++-------
> gas/config/obj-macho.h | 3 +
> 4 files changed, 262 insertions(+), 34 deletions(-)
>
> diff --git a/bfd/mach-o.c b/bfd/mach-o.c
> index a1f6596..493116a 100644
> --- a/bfd/mach-o.c
> +++ b/bfd/mach-o.c
> @@ -2067,6 +2067,33 @@ bfd_mach_o_build_seg_command (const char *segment,
> return TRUE;
> }
>
> +/* Count the number of indirect symbols in the image.
> + Requires that the sections are in their final order. */
> +
> +static unsigned int
> +bfd_mach_o_count_indirect_symbols (bfd *abfd, bfd_mach_o_data_struct *mdata)
> +{
> + unsigned int i;
> + unsigned int nisyms = 0;
> +
> + for (i = 0; i < mdata->nsects; ++i)
> + {
> + bfd_mach_o_section *sec = mdata->sections[i];
> +
> + switch (sec->flags & BFD_MACH_O_SECTION_TYPE_MASK)
> + {
> + case BFD_MACH_O_S_NON_LAZY_SYMBOL_POINTERS:
> + case BFD_MACH_O_S_LAZY_SYMBOL_POINTERS:
> + case BFD_MACH_O_S_SYMBOL_STUBS:
> + nisyms += bfd_mach_o_section_get_nbr_indirect (abfd, sec);
> + break;
> + default:
> + break;
> + }
> + }
> + return nisyms;
> +}
> +
ok.
> static bfd_boolean
> bfd_mach_o_build_dysymtab_command (bfd *abfd,
> bfd_mach_o_data_struct *mdata,
> @@ -2123,9 +2150,11 @@ bfd_mach_o_build_dysymtab_command (bfd *abfd,
> dsym->nundefsym = 0;
> }
>
> + dsym->nindirectsyms = bfd_mach_o_count_indirect_symbols (abfd, mdata);
> if (dsym->nindirectsyms > 0)
> {
> unsigned i;
> + unsigned n;
>
> mdata->filelen = FILE_ALIGN (mdata->filelen, 2);
> dsym->indirectsymoff = mdata->filelen;
> @@ -2134,11 +2163,37 @@ bfd_mach_o_build_dysymtab_command (bfd *abfd,
> dsym->indirect_syms = bfd_zalloc (abfd, dsym->nindirectsyms * 4);
> if (dsym->indirect_syms == NULL)
> return FALSE;
> -
> - /* So fill in the indices. */
> - for (i = 0; i < dsym->nindirectsyms; ++i)
> +
> + n = 0;
> + for (i = 0; i < mdata->nsects; ++i)
> {
> - /* TODO: fill in the table. */
> + bfd_mach_o_section *sec = mdata->sections[i];
> +
> + switch (sec->flags & BFD_MACH_O_SECTION_TYPE_MASK)
> + {
> + case BFD_MACH_O_S_NON_LAZY_SYMBOL_POINTERS:
> + case BFD_MACH_O_S_LAZY_SYMBOL_POINTERS:
> + case BFD_MACH_O_S_SYMBOL_STUBS:
> + {
> + bfd_mach_o_indirect_sym *isym = sec->indirectsyms;
> + if (isym == NULL)
> + break;
> + /* Record the starting index in the reserved1 field. */
> + sec->reserved1 = n;
> + do
> + {
> + dsym->indirect_syms[n] = isym->sym->symbol.udata.i;
> + n++;
> + /* Final safety net. */
> + if (n > dsym->nindirectsyms)
> + abort ();
Use bfd_assert instead.
Note that we should take care of local and absolute symbols.
> + }
> + while ((isym = isym->next) != NULL);
> + }
> + break;
> + default:
> + break;
> + }
> }
> }
>
> diff --git a/bfd/mach-o.h b/bfd/mach-o.h
> index ca810a0..31a4095 100644
> --- a/bfd/mach-o.h
> +++ b/bfd/mach-o.h
> @@ -63,6 +63,12 @@ typedef struct bfd_mach_o_section
>
> /* Corresponding bfd section. */
> asection *bfdsection;
> +
> + /* Linked list of indirect symbols for this section (only applies to
> + stub and reference sections). */
> + struct bfd_mach_o_indirect_sym *indirectsyms;
> + /* Pointer to the last indirect, to save reversing the list. */
> + struct bfd_mach_o_indirect_sym *lastindirectsym;
>
> /* Simply linked list. */
> struct bfd_mach_o_section *next;
> @@ -117,6 +123,16 @@ typedef struct bfd_mach_o_asymbol
> }
> bfd_mach_o_asymbol;
>
> +/* An element in a list of indirect symbols the root of which
> + is "indirectsyms" in the section to which they apply. */
> +
> +typedef struct bfd_mach_o_indirect_sym
> +{
> + struct bfd_mach_o_indirect_sym *next;
> + struct bfd_mach_o_asymbol *sym;
> +}
> +bfd_mach_o_indirect_sym;
> +
> /* The symbol table is sorted like this:
> (1) local.
> (otherwise in order of generation)
> diff --git a/gas/config/obj-macho.c b/gas/config/obj-macho.c
> index 43f4fba..5d82977 100644
> --- a/gas/config/obj-macho.c
> +++ b/gas/config/obj-macho.c
> @@ -39,6 +39,7 @@
>
> #include "as.h"
> #include "subsegs.h"
> +#include "struc-symbol.h" /* For local symbol stuff in frob_label/symbol. */
> #include "symbols.h"
> #include "write.h"
> #include "mach-o.h"
> @@ -1032,6 +1033,7 @@ obj_mach_o_set_symbol_qualifier (symbolS *sym, int type)
>
> case OBJ_MACH_O_SYM_PRIV_EXT:
> s->n_type |= BFD_MACH_O_N_PEXT ;
> + s->n_desc &= ~LAZY; /* The native tool swithes this off too. */
> /* We follow the system tools in marking PEXT as also global. */
> /* Fall through. */
Unrelated chunk ?
>
> @@ -1131,13 +1133,74 @@ obj_mach_o_sym_qual (int ntype)
> demand_empty_rest_of_line ();
> }
>
> -/* Dummy function to allow test-code to work while we are working
> - on things. */
> -
> static void
> -obj_mach_o_placeholder (int arg ATTRIBUTE_UNUSED)
> +obj_mach_o_indirect_symbol (int arg ATTRIBUTE_UNUSED)
> {
> - ignore_rest_of_line ();
> + bfd_mach_o_section *sec = bfd_mach_o_get_mach_o_section (now_seg);
> + unsigned lazy = 0;
> +
> +#ifdef md_flush_pending_output
> + md_flush_pending_output ();
> +#endif
> +
> + switch (sec->flags & BFD_MACH_O_SECTION_TYPE_MASK)
> + {
> + case BFD_MACH_O_S_SYMBOL_STUBS:
> + case BFD_MACH_O_S_LAZY_SYMBOL_POINTERS:
> + lazy = LAZY;
> + /* Fall through. */
> + case BFD_MACH_O_S_NON_LAZY_SYMBOL_POINTERS:
> + {
> + bfd_mach_o_indirect_sym *isym;
> + char *name = input_line_pointer;
> + char c = get_symbol_end ();
> + symbolS *sym = symbol_find_or_make (name);
> + unsigned int elsize =
> + bfd_mach_o_section_get_entry_size (stdoutput, sec);
> +
> + if (elsize == 0)
> + {
> + as_bad (_("attempt to add an indirect_symbol to a stub or"
> + " reference section with a zero-sized element at %s"),
> + name);
> + *input_line_pointer = c;
> + ignore_rest_of_line ();
> + return;
> + }
> +
> + *input_line_pointer = c;
> +
> + /* This should be allocated in bfd, since it is owned there. */
> + isym = (bfd_mach_o_indirect_sym *)
> + bfd_zalloc (stdoutput, sizeof (bfd_mach_o_indirect_sym));
> + if (isym == NULL)
> + abort ();
> +
> + isym->sym = (bfd_mach_o_asymbol *) symbol_get_bfdsym (sym);
> + if (sec->indirectsyms == NULL)
> + sec->indirectsyms = isym;
> + else
> + sec->lastindirectsym->next = isym;
> + sec->lastindirectsym = isym;
> +
> + /* We put in the lazy flag, it will get reset if the symbols is later
> + defined, or if the symbol becomes private_extern. */
> + if (isym->sym->symbol.section == bfd_und_section_ptr
> + && ! (isym->sym->n_type & BFD_MACH_O_N_PEXT))
> + {
> + isym->sym->n_desc = lazy;
> + isym->sym->symbol.udata.i = SYM_MACHO_FIELDS_NOT_VALIDATED;
> + }
> + }
> + break;
> +
> + default:
> + as_bad (_("an .indirect_symbol must be in a symbol pointer"
> + " or stub section."));
> + ignore_rest_of_line ();
> + return;
> + }
> + demand_empty_rest_of_line ();
> }
>
> const pseudo_typeS mach_o_pseudo_table[] =
> @@ -1231,7 +1294,7 @@ const pseudo_typeS mach_o_pseudo_table[] =
> {"no_dead_strip", obj_mach_o_sym_qual, OBJ_MACH_O_SYM_NO_DEAD_STRIP},
> {"weak", obj_mach_o_sym_qual, OBJ_MACH_O_SYM_WEAK}, /* ext */
>
> - {"indirect_symbol", obj_mach_o_placeholder, 0},
> + {"indirect_symbol", obj_mach_o_indirect_symbol, 0},
>
> /* File flags. */
> { "subsections_via_symbols", obj_mach_o_fileprop,
> @@ -1270,15 +1333,33 @@ obj_mach_o_type_for_symbol (bfd_mach_o_asymbol *s)
>
> void obj_macho_frob_label (struct symbol *sp)
> {
> - bfd_mach_o_asymbol *s = (bfd_mach_o_asymbol *) symbol_get_bfdsym (sp);
> - /* This is the base symbol type, that we mask in. */
> - unsigned base_type = obj_mach_o_type_for_symbol (s);
> - bfd_mach_o_section *sec = bfd_mach_o_get_mach_o_section (s->symbol.section);
> + bfd_mach_o_asymbol *s;
> + unsigned base_type;
> + bfd_mach_o_section *sec;
> int sectype = -1;
>
> + /* Leave local symbols alone (unless they've already been made into a real
> + one). */
> +
> + if (0 && sp->bsym == NULL)
> + {
> + if (((struct local_symbol *) sp)->lsy_section != reg_section)
> + return;
> + else
> + /* We have a local which has been added to the symbol table, so carry
> + on with the checking. */
> + sp = ((struct local_symbol *) sp)->u.lsy_sym;
> + }
Don't let 'if (0)' code.
Please, use macro accessor instead of '((struct local_symbol *) sp)->lsy_section != reg_section'.
> +
> + s = (bfd_mach_o_asymbol *) symbol_get_bfdsym (sp);
> + /* Leave debug symbols alone. */
> if ((s->n_type & BFD_MACH_O_N_STAB) != 0)
> - return; /* Leave alone. */
> -
> + return;
> +
> + /* This is the base symbol type, that we mask in. */
> + base_type = obj_mach_o_type_for_symbol (s);
> +
> + sec = bfd_mach_o_get_mach_o_section (s->symbol.section);
> if (sec != NULL)
> sectype = sec->flags & BFD_MACH_O_SECTION_TYPE_MASK;
>
> @@ -1307,34 +1388,50 @@ void obj_macho_frob_label (struct symbol *sp)
> int
> obj_macho_frob_symbol (struct symbol *sp)
> {
> - bfd_mach_o_asymbol *s = (bfd_mach_o_asymbol *) symbol_get_bfdsym (sp);
> - unsigned base_type = obj_mach_o_type_for_symbol (s);
> - bfd_mach_o_section *sec = bfd_mach_o_get_mach_o_section (s->symbol.section);
> + bfd_mach_o_asymbol *s;
> + unsigned base_type;
> + bfd_mach_o_section *sec;
> int sectype = -1;
> -
> +
> + /* Leave local symbols alone (unless they've already been made into a real
> + one). */
> +
> + if (0 && sp->bsym == NULL)
> + {
> + if (((struct local_symbol *) sp)->lsy_section != reg_section)
> + return 0;
> + else
> + /* We have a local which has been added to the symbol table, so carry
> + on with the checking. */
> + sp = ((struct local_symbol *) sp)->u.lsy_sym;
> + }
Likewise.
> +
> + s = (bfd_mach_o_asymbol *) symbol_get_bfdsym (sp);
> + /* Leave debug symbols alone. */
> + if ((s->n_type & BFD_MACH_O_N_STAB) != 0)
> + return 0;
> +
> + base_type = obj_mach_o_type_for_symbol (s);
> + sec = bfd_mach_o_get_mach_o_section (s->symbol.section);
> if (sec != NULL)
> sectype = sec->flags & BFD_MACH_O_SECTION_TYPE_MASK;
>
> - if ((s->n_type & BFD_MACH_O_N_STAB) != 0)
> - return 0; /* Leave alone. */
> - else if (s->symbol.section == bfd_und_section_ptr)
> + if (s->symbol.section == bfd_und_section_ptr)
> {
> /* ??? Do we really gain much from implementing this as well as the
> mach-o specific ones? */
> if (s->symbol.flags & BSF_WEAK)
> s->n_desc |= BFD_MACH_O_N_WEAK_REF;
>
> - /* Undefined references, become extern. */
> - if (s->n_desc & REFE)
> - {
> - s->n_desc &= ~REFE;
> - s->n_type |= BFD_MACH_O_N_EXT;
> - }
> -
> - /* So do undefined 'no_dead_strip's. */
> - if (s->n_desc & BFD_MACH_O_N_NO_DEAD_STRIP)
> - s->n_type |= BFD_MACH_O_N_EXT;
> -
> + /* Undefined syms, become extern. */
> + s->n_type |= BFD_MACH_O_N_EXT;
> + S_SET_EXTERNAL (sp);
> + }
> + else if (s->symbol.section == bfd_com_section_ptr)
> + {
> + /* ... so to comm. */
> + s->n_type |= BFD_MACH_O_N_EXT;
> + S_SET_EXTERNAL (sp);
> }
> else
> {
> @@ -1353,6 +1450,7 @@ obj_macho_frob_symbol (struct symbol *sp)
> {
> /* Anything here that should be added that is non-standard. */
> s->n_desc &= ~BFD_MACH_O_REFERENCE_MASK;
> + s->symbol.udata.i = SYM_MACHO_FIELDS_NOT_VALIDATED;
> }
> else if (s->symbol.udata.i == SYM_MACHO_FIELDS_NOT_VALIDATED)
> {
> @@ -1388,6 +1486,62 @@ obj_macho_frob_symbol (struct symbol *sp)
> return 0;
> }
>
> +void
> +obj_mach_o_frob_section (asection *sec)
> +{
> + bfd_vma sect_size = bfd_section_size (stdoutput, sec);
> + bfd_mach_o_section *ms = bfd_mach_o_get_mach_o_section (sec);
> +
> + /* Process indirect symbols to determine if we have errors there. */
> +
> + switch (ms->flags & BFD_MACH_O_SECTION_TYPE_MASK)
> + {
> + case BFD_MACH_O_S_SYMBOL_STUBS:
> + case BFD_MACH_O_S_LAZY_SYMBOL_POINTERS:
> + case BFD_MACH_O_S_NON_LAZY_SYMBOL_POINTERS:
> + {
> + unsigned int nactual;
> + unsigned int ncalc;
> + bfd_mach_o_indirect_sym *isym = ms->indirectsyms;
> + unsigned long eltsiz =
> + bfd_mach_o_section_get_entry_size (stdoutput, ms);
> +
> + /* If we somehow added indirect symbols to a section with a zero
> + entry size, we're dead ... */
> + if (eltsiz == 0 && isym != NULL)
> + abort ();
gas_assert.
> +
> + ncalc = (unsigned int) (sect_size / eltsiz);
> +
> + nactual = 0;
> + if (isym != NULL)
> + do
> + {
> + /* Count it. */
> + nactual++;
> + if (isym->sym->symbol.section == bfd_und_section_ptr)
> + {
> + /* If the referenced symbol is undefined, make
> + it extern. */
> + isym->sym->n_type |= BFD_MACH_O_N_EXT;
> + isym->sym->symbol.flags |= BSF_GLOBAL;
> + }
> + }
> + while ((isym = isym->next) != NULL);
> +
> + if (nactual != ncalc)
> + as_bad (_("there %s %d indirect_symbol%sin section %s but"
> + " %d %s expected"), (nactual == 1)?"is":"are", nactual,
"is":"are" don't make sense with other languages !
> + (nactual == 1)?" ":"s ", sec->name, ncalc,
> + (ncalc == 1)?"is":"are");
> + }
> + break;
> +
> + default:
> + break;
> + }
> +}
> +
> /* Support stabs for mach-o. */
>
> void
> diff --git a/gas/config/obj-macho.h b/gas/config/obj-macho.h
> index cbc3a4f..ceb1097 100644
> --- a/gas/config/obj-macho.h
> +++ b/gas/config/obj-macho.h
> @@ -62,6 +62,9 @@ extern void obj_macho_frob_label (struct symbol *);
> #define obj_frob_symbol(s, punt) punt = obj_macho_frob_symbol(s)
> extern int obj_macho_frob_symbol (struct symbol *);
>
> +#define obj_frob_section(s) obj_mach_o_frob_section(s)
> +void obj_mach_o_frob_section (asection *);
> +
> #define EMIT_SECTION_SYMBOLS 0
>
> #define OBJ_PROCESS_STAB(SEG,W,S,T,O,D) obj_mach_o_process_stab(W,S,T,O,D)
Tristan.