This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch mach-o, indirect symbols 1/2] support indirect symbols in mach-o.


Hi Tristan,

I've amended and applied (together with its tests) with the additional comments as below.

Note that I updated the (c) years in the altered files, but I did NOT rotate the ChangeLogs.

Iain

On 2 Jan 2012, at 11:13, Tristan Gingold wrote:

Ok with the suggested changes (mostly stylistic). Bracketed points are not required to be addressed for the commit.
+
+      if ((s->n_type & BFD_MACH_O_N_TYPE) == BFD_MACH_O_N_INDR)

[ Maybe we should create a macro to test for indirect symbols ]

done.


+	/* A pointer to the referenced symbol will be stored in the udata
+	   field.  Use that to find the string index.  */
+	s->symbol.value =
+	    ((bfd_mach_o_asymbol *)s->symbol.udata.p)->symbol.udata.i;

[ One issue is that we don't follow the BFD convention for indirect symbols. To be fixed later ]

that's a tricky one - we need the 'normal' symbols ahead of the indirects (so we'd have to swap in and out).


[ One question: does it make sense to output the indirect name here and therefore avoiding taking care of indirect symbols while sorting ? ]

I am not sure I follow the question - we are constrained to do things in a particular order by needing certain dependent information.


to construct and order the symtab - we need the sections.
to construct the indirect symbol dependent indices - we need the symbols sorted.


to construct the indirect symbol *value* we need the string table offset for the dependent symbol (which is only available when the symbol table is written). Note that this is academic when we write a dysymtab - since we don't then write 'bare' indirect symbols. If we do not write the dysymtab - then the 'normal' symbols precede the indirect ones which means that we get the *value* just in time.

+ }

Please don't use the raw buffer buf. Instead use mach_o_dylib_module[_64]_external. I know it is boring because of 32/64.

done.


+
+	  bfd_h_put_32 (abfd, cmd->indirect_syms[i], &raw);

[ Ditto (although might be considered as too trivial) ]

left as is, because there is not an entry in external.h for it...


+ bfd_h_put_32 (abfd, v, raw);

[ Ditto ]

likewise.


+ if (type == BFD_MACH_O_N_INDR) return 3;

GNU style issue: not on the same line!

sorry, dunno what I was thinking of here...


+ qsort (symbols, (size_t) nin, sizeof (void *), bfd_mach_o_cf_symbols);

[ Unfortunately qsort is not stable! ]

indeed, which is why there's a specific sort decider for each type - if you believe I've missed one (or that it's not adequate) then I guess we could switch to mergesort.


===
applied patch attached FTR.

Attachment: 12010311-write-dysymtab.txt
Description: Text document



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]