This is just a small feature request to help people notice mistakes. When using Makefiles to build things, it's easy to write rules like: linker_script.ld: linker_script.ld.S <preprocess with CPP> my.elf: $(myobjects) $(somelibrary) linker_script.ld $(LD) $(LDFLAGS) -o $@ -T linker_script.ld $^ There's a subtle error in the above: sincer linker_script.ld is part of the prerequisites, it will be part of the $^ variable expansion, despite already being listed explicitly for the -T flag. This means the linker will both use it as the default linker script and then use it again to "augment" that script. This will often not lead to an obvious error but cause all kinds of subtle hijinks with symbol placement. The resulting binary may still run and contain hard to understand rare errors. I don't think there could ever be a valid use case where you'd want to pass the same file as an argument to -T and as an augment linker script later on, so could you just add a check to error out somewhere if that happens? That could save the people running into this a lot of headache.
(In reply to Julius Werner from comment #0) Hi Julius, > I don't think there could ever be a valid use case where you'd want to pass > the same file as an argument to -T and as an augment linker script later on, > so could you just add a check to error out somewhere if that happens? That > could save the people running into this a lot of headache. This seems like a reasonable request. Would you like to have a go at implementing the idea yourself and submitting a patch ? [A not so subtle attempt to get a new contributor interested in the binutils...] You could record the name of the last linker script parsed with the -T option by adding code to ld/lexsup.c:parse_args(), and then check it against the filenames that are processed by ld/ldfile.c:ldfile_try_open_bfd. If you are feeling particularly clever you could record *all* of the filenames used with -T as there could be multiple instances of it. Cheers Nick
Created attachment 11789 [details] proposed fix Hi Nick, Okay, I looked into it and I think I found a reasonably clean way to do it (just checking that no linker script is ever parsed twice, because that just wouldn't make sense in any situation). However, I'm not quite sure how to submit my patch... I can't really find contributing guidelines for binutils anywhere. Can you link me to the right place or explain the process? (Attaching the patch here for lack of a better way to submit it.)
(In reply to Julius Werner from comment #2) Hi Julius, > Okay, I looked into it and I think I found a reasonably clean way to do it > (just checking that no linker script is ever parsed twice, because that just > wouldn't make sense in any situation). Thanks. I did feel I was being a bit cheeky by asking you to do this, but it is nice to see that you went ahead anyway! :-) > However, I'm not quite sure how to > submit my patch... I can't really find contributing guidelines for binutils > anywhere. Can you link me to the right place or explain the process? Sure. There are some notes in the binutils/MAINTAINERS file, but here is a quick summary: * Patches should be created according to the GNU Coding Standard: https://www.gnu.org/prep/standards/ * They should be submitted to the binutils mailing list for review. (binutils@sourceware.org) * They should be tested before hand. (You would be surprised how often this does not appear to happen). * For "legally significant" patches a copyright assignment needs to be on file with the FSF before the patch can be accepted. https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html#Legally-Significant In order to obtain a copyright assignment, one of these forms needs to be completed and sent to the FSF: http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=tree;f=doc/Copyright;h=22db9a802b4da96ad455cd933351c1359108f95d;hb=HEAD I assume that you are familiar with the concept of patch review and resubmission, so I will not go into that. > Attaching the patch here for lack of a better way to submit it. Which is fine in this case. I do not see a copyright assignment on file for you or Chromium, but I think that in this particular case it is safe to consider the patch as not being legally significant. So I have gone ahead and tested your patch, found no problems and applied it to the sources. Cheers Nick
> So I have gone > ahead and tested your patch, found no problems and applied it to the sources. No I haven't. It is always the way. The tests are running smoothly, I assume that everything will be OK, and so I update the PR, and then a bug turns up. *sigh* So the patch is not in (yet) but I am investigating to see if I can fix it.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6ec6968b1b259948ba42f0a47a3da048377058bc commit 6ec6968b1b259948ba42f0a47a3da048377058bc Author: Nick Clifton <nickc@redhat.com> Date: Wed May 22 15:58:57 2019 +0100 Have the linker report an error if the same script is used twice. PR 24576 * ld/ldfile.c: (ldfile_open_command_file_1): Add new parameter - is_script. If true check that the file has not already been parsed as a linker script. (ldfile_open_script_file): New function. (ldfile_try_open_bfd): Use the new function in place of ldfile_open_command_line. * ldmain.c (main): Likewise. * lexsup.c (parse_args): Use the new function for opening linker scripts with the -T option. * ldfile.h (ldfile_open_script_file): Add prototype.
Right - it really has gone in this time. The problem was that ldfile_open_command_file_1() can legitimately be called multiple times for the same input file. This happens for example when the linker is processing an archive. So it was necessary to distinguish between attempts to open a file as a user-specified script file, (or an unrecognised input file) and other types of access. I added the code to do this, and the check now appears to working properly.
Great, thanks for taking care of it!
This patch results in testsuite failures such as: /home/alan/build/gas/all/ld/tmpdir/ld/collect-ld: error: linker script file '/usr/local/lib64/libgcc_s.so' appears multiple times collect2: error: ld returned 1 exit status FAIL: bootstrap There isn't anything wrong with specifying a shared library more than once on the command line, and sometimes like the example above, the library is installed as a script that loads both a static archive and the actual shared library.
The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=82d7a6f4e3ccb3d714b5beb03eeb24f7356d4380 commit 82d7a6f4e3ccb3d714b5beb03eeb24f7356d4380 Author: Alan Modra <amodra@gmail.com> Date: Thu May 23 10:22:56 2019 +0930 Re: Have the linker report an error if the same script is used twice git commit 6ec6968b1b2 results in ... error: linker script file '/usr/local/lib64/libgcc_s.so' appears multiple times collect2: error: ld returned 1 exit status FAIL: bootstrap This patch changes things so that an error is given only when a -T script or the default script is invoked more than once. I'm still a little nervous that we match script file names, not the entire path. PR 24576 * ldfile.c (enum script_open_style): New. (struct script_name_list): New. (ldfile_open_command_file_1): Take a script_open_style param rather than booleans. Adjust callers. Only fail when -T or default -T script is invoked twice. (ldfile_try_open_bfd): Revert last change.
Hopefully now fixed.