Home Original page

[PATCH v2] ld: Find files relative to the current linker script for INPUT() and GROUP()

Fangrui Song maskray@google.com
Mon Apr 20 22:54:49 GMT 2020 
On 2020-04-20, Nick Clifton wrote:
>Hi Fangrui,
>
>> 	PR ld/25806
>> 	* ldlang.h (struct lang_input_statement_struct): Add extra_search_path.
>> 	* ldlang.c (current_input_file): New.
>> 	(ldirname): New.
>> 	(new_afile): Add from_filename parameter. Set extra_search_path.
>> 	(lang_add_input_file): Pass current_input_file to new_afile.
>> 	(load_symbols): Set current_input_file.
>
>Note - adding a new feature like this definitely needs some documentation
>added to ld/ld.texi, an entry in ld/NEWS

Added some contents to ld/ld.texi and ld/NEWS in PATCH v2.

> and ideally one or more new tests>added to ld/testsuite/ld-scripts.

....... it is unfortunate that I cannot find *ANY* test related to
INPUT/GROUP search rules (ld/testsuite/ld-{i386,x86-64}/pr24276.dso have the
substring "INPUT(" but they are not related to search rules)

I do have some personal tests, but I don't know how to place them within the current testing framework (ld/testsuite/ld-elf/* if I understand correctly)

Scroll down to the bottom to see my tests.

>> diff --git a/ld/ChangeLog b/ld/ChangeLog
>> index dffd363494..d03acbbc28 100644
>
>FYI - there is no need to include the ChangeLog as part of the context
>diff.  Providing it as plain text, as you have done above, is sufficient.
>Indeed having a diff to a changelog entry often makes the patch fail to
>apply cleanly as the changelogs do get updated quite often.

Ack. Dropped ld/ChangeLog in PATCH v2.

>> +      /* If extra_search_path is set, entry->filename is a relative path.
>> +         Search the directory of the current linkerscript script before
>> +         searching other paths. */
>
>s/linkerscript/linker script/

Fixed.

>> +      if (entry->extra_search_path)
>> +        {
>> +          char *path = concat (entry->extra_search_path, slash, entry->filename,
>> +                               (const char *)0);
>
>I prefer NULL to (const char *)0 myself.

If I interpret Alan's comment correctly, I will keep (const char *)0 as is.

>
>> +          if (ldfile_try_open_bfd (path, entry)) {
>
>Formatting: Please put the { on a separate line.

Fixed.

>> +            entry->filename = path;
>> +            entry->flags.search_dirs = FALSE;
>> +            return;
>> +          }There is a memory leak here - path is not freed.
>> +        }

Added `free (path) to the else branch.  I don't know how to address the then
branch. Near line 100, we have such code:

   if (name[0] == '=')
     new_dirs->name = concat (ld_sysroot, name + 1, (const char *) NULL);
   else if (CONST_STRNEQ (name, "$SYSROOT"))
     new_dirs->name = concat (ld_sysroot, name + strlen ("$SYSROOT"), (const char *) NULL);
   else
     new_dirs->name = xstrdup (name);

I follow suit and assign an allocated string to extra_search_path.

>
>
>>  	 within the sysroot subdirectory.)  */
>>        unsigned int outer_sysrooted = input_flags.sysrooted;
>>        input_flags.sysrooted = 0;
>
>This code fragment made me wonder.  Have you tested your patch
>in a sys-rooted environment ?  Does it still work ?
>
>Plus that reminds me - which target(s) have you used to check this patch ?

>Cheers
>  Nick

Linux x86-64. The tests I want to use are something like the following,
a free-form shell style testing.

# REQUIRES: x86
## For a relative pathname in INPUT() or GROUP(), the parent directory of
## the current linker script has priority over current working directory and -L.

# RUN: rm -rf %t.dir && mkdir %t.dir && cd %t.dir

# RUN: mkdir dir
# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o a.o
# RUN: echo '.globl b, cwd; b: cwd:' | llvm-mc -filetype=obj -triple=x86_64 - -o b.o
# RUN: echo '.globl b, dir; b: dir:' | llvm-mc -filetype=obj -triple=x86_64 - -o dir/b.o
# RUN: ar rc libb.a b.o
# RUN: ar rc dir/libb.a dir/b.o

## A relative pathname is relative to the parent directory of the current linker script.
## The directory has priority over current working directory and -L.
# RUN: echo 'INPUT(libb.a)' > dir/relative.lds
# RUN: ld-new -L. a.o dir/relative.lds -o relative
# RUN: nm relative | FileCheck --check-prefix=DIR %s
## GROUP() uses the same search order.
# RUN: echo 'GROUP(libb.a)' > dir/relative1.lds
# RUN: ld-new -L. a.o dir/relative1.lds -o relative1
# RUN: nm relative1 | FileCheck --check-prefix=DIR %s

# DIR: T dir

## -l does not use the special rule.
# RUN: echo 'INPUT(-lb)' > dir/cwd.lds
# RUN: ld-new -L. a.o dir/cwd.lds -o cwd
# RUN: nm cwd | FileCheck --check-prefix=CWD %s
# RUN: echo 'GROUP(-lb)' > dir/cwd1.lds
# RUN: ld-new -L. a.o dir/cwd1.lds -o cwd1
# RUN: nm cwd1 | FileCheck --check-prefix=CWD %s

# CWD: T cwd

## If the parent directory of the current linker script does not contain the file,
## fall back to the current working directory.
# RUN: cp libb.a libc.a
# RUN: echo 'INPUT(libc.a)' > dir/fallback.lds
# RUN: ld-new a.o dir/fallback.lds -o /dev/null

.globl _start
_start:
   call b

# The above is adapted from https://reviews.llvm.org/D77779?id=258631


The main components of the testing framework are

* a tool to invoke RUN lines as shell commands
* a meta grep (FileCheck) which checks whether its stdin contains certain input lines with flexible patterns
   (https://llvm.org/docs/CommandGuide/FileCheck.html)


This is actually somethine I wanted to propose on this list :) We need
to reimplement the meta grep tool and the testing framework.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ld-Find-files-relative-to-the-current-linker-script-.patch
Type: text/x-diff
Size: 7914 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20200420/52c542c9/attachment.bin>
More information about the Binutils mailing list