From: "Joseph S. Myers" <joseph@codesourcery.com>
To: Bernd Schmidt <bernds@codesourcery.com>
Cc: binutils@sources.redhat.com, Paul Brook <paul@codesourcery.com>
Subject: Re: Patch: Add c6x-uclinux support
Date: Mon, 28 Mar 2011 22:34:00 -0000 [thread overview]
Message-ID: <Pine.LNX.4.64.1103282214490.30472@digraph.polyomino.org.uk> (raw)
In-Reply-To: <4D7A0A32.7070308@codesourcery.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
next prev parent reply other threads:[~2011-03-28 22:34 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-11 11:42 Bernd Schmidt
2011-03-28 22:34 ` Joseph S. Myers [this message]
2011-03-28 23:18 ` Bernd Schmidt
2011-03-29 16:13 ` Joseph S. Myers
2011-03-29 22:59 ` Bernd Schmidt
2011-03-29 23:42 ` Joseph S. Myers
2011-03-30 8:23 ` Bernd Schmidt
2011-03-30 10:14 ` Joseph S. Myers
2011-03-30 11:12 ` Bernd Schmidt
2011-03-30 11:19 ` Joseph S. Myers
2011-03-30 18:46 ` Bernd Schmidt
2011-03-30 19:34 ` Joseph S. Myers
2011-03-31 14:40 ` Tristan Gingold
2011-03-31 16:14 ` Bernd Schmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.64.1103282214490.30472@digraph.polyomino.org.uk \
--to=joseph@codesourcery.com \
--cc=bernds@codesourcery.com \
--cc=binutils@sources.redhat.com \
--cc=paul@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).