Re: Patch: Add c6x-uclinux support

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Fri, 11 Mar 2011, Bernd Schmidt wrote:

> Also included in this patch is support for a new assembler directive,
> ".scomm", used to better support small-data common symbols as required
> by the ABI. There's also a small assembler bugfix for -mgenerate-rel.

Why exactly is a .scommon section needed in addition to .bss and .far - 
how does it differ from those two sections?

Also, this directive appears to generate SHN_TIC6X_SCOMMON in object 
files.  I don't see any sign of this in the ABI ("There are no 
processor-specific special section indexes defined.") - exactly what 
specification of it has been agreed with TI?  We can't put 
processor-specific section indices in object files unless they are defined 
by the ABI (including having a specification agreed for the next ABI 
version).  I also notice that the diagnostics in .scomm parsing often fail 
to follow the GNU Coding Standards (other diagnostics in tc-tic6x.c make 
sure to avoid initial capital letters or trailing '.'; they also avoid the 
grave accent as an opening quote, although this last style point, though 
consistent with GCC, is outside the GNU Coding Standards).

In addition, .scomm handling should have assembler testcases for both 
correct and invalid uses (covering the different diagnostics).

I haven't yet reviewed the whole patch, but some other comments and 
questions:

* There is a reference to R_386_COPY in the patch, which is clearly wrong.

* R_C6000_SBR_GOT32, added to include/elf/tic6x.h by the patch, is not the 
name agreed for the next version of the ABI; that's R_C6000_EHTYPE.

* I presume semantics for R_C6000_EHTYPE, R_C6000_PCR_H16, and 
R_C6000_PCR_L16 are to be added in subsequent patches, since only the 
definitions and nothing else are added in this patch.  (HOWTO entries for 
new relocations are useful even without more substantive linker support, 
since they are used by objdump when processing objects - maybe produced 
with the TI tools - that contain the relocations.)

* I think the static functions in elf32-tic6x.c should be consistently 
named with an elf32_tic6x_ prefix (e.g. using_dsbt, install_rela, 
make_got_dynreloc).

* What is the purpose of the ELF_MAXPAGESIZE change (and the logic behind 
the choice of the new value)?  I don't think the value is of much 
significance, but it would be good to know why the change was needed.

* There is a reference to a symbol "__DYNAMIC", which I think should be 
"_DYNAMIC".  (bfin has a reference to "__DYNAMIC", but I think that's 
because it deviates from the gABI in having a leading underscore on symbol 
names, unlike C6X.)

* Why does elf32_tic6x_gc_sweep_hook handle R_C6000_SBR_GOT_U15_W 
differently from R_C6000_SBR_GOT_L16_W and R_C6000_SBR_GOT_H16_W?  I 
haven't tried to construct a testcase showing this is a problem, but it 
seems strange for it to do so without a comment.

* There is a test for the undefined weak symbol handling for 
R_C6000_PCR_S21, in the case where the instruction has the specified form 
to be converted to a branch to the return address; tests for the other 
cases with required semantics (absolute and SB-relative relocations 
resolving to 0 and to the static base respectively) would be good.

-- 
Joseph S. Myers
joseph@codesourcery.com