This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 1/5] New target port: Andes 'nds32'. (bfd)
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Kuan-Lin Chen <kuanlinchentw at gmail dot com>
- Cc: <binutils at sourceware dot org>
- Date: Tue, 9 Jul 2013 16:19:41 +0000
- Subject: Re: [PATCH 1/5] New target port: Andes 'nds32'. (bfd)
- References: <CAJr6u0h4WK8BHp1+N1E9WKeBuX+5nFVWrprKDEHMu14EL2tPAw at mail dot gmail dot com>
This patch has some GPLv2 notices in it. Please change them to GPLv3.
Please use the version of the license notice that says "see
<http://www.gnu.org/licenses/>" rather than giving an FSF postal address.
(We really ought to convert existing files in binutils....)
As far as I can see, nds32_elfver_strtab could be const (i.e. "static
const char *const nds32_elfver_strtab[]" - make the array itself const,
not just the pointer targets). The sec_name array in
nds32_elf_final_sda_base looks like it could be static const. Probably
other arrays as well should be static const.
Do you really need your target options to go in global variables (with
names that don't mention the target at all - remember that it's possible
to build BFD to support multiple targets, so all exported symbols need to
mention the target name to avoid conflicts)? There are lots of options in
e.g. struct elf32_arm_link_hash_table that seem analogous to those you are
putting in global variables, which suggests you should be able to put
yours in the corresponding structure. (armelf.em then calls
bfd_elf32_arm_set_target_relocs to pass such configuration information to
BFD.) I don't know the use of align_addend, sdata_range,
sdata_init_range, but certainly they can't be exported with those names
("nm" on elf32-nds32.o should not reveal any exported symbols whose names
don't mention nds32), and in general it's better for modifiable data to be
associated in some way with a BFD instance rather than static if possible.
nds32_insertion_sort uses alloca, but you could have too many relocations
to fit on the stack - whenever using alloca you need to fallback to
xmalloc if the size is too big (or simply use xmalloc unconditionally), to
avoid stack overflow.
You have a "FIXME: This should not be a static variable.". So fix it.
(Review all such comments in general, at least to consider whether they do
still reflect a desired change and whether that change would be easy to
make now.)
--
Joseph S. Myers
joseph@codesourcery.com