[PATCH] allow empty string as argument to -Map

[PATCH] allow empty string as argument to -Map

[Rasmus Villemoes]

In meta-build systems like Yocto, it's rather easy to add flags like
-fdata-sections, -ffunction-sections, -Wl,--gc-sections to global
CFLAGS/LDFLAGS. But when digging into what effect those had on the
build of a particular package, it would be nice to also have a map
file automatically generated. That's currently a bit harder, since it
requires patching the build system of each individual package to add
something like -Wl,-Map=$@.map somewhere.

My first instinct was to make the argument to -Map optional, but that
will of course break anybody that uses the form "-Map foo.map" rather
than "-Map=foo.map".

So, as the next best thing, use a previously bogus value (the empty
string) as a sentinel to indicate "use the output filename + .map".

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 ld/ld.texi  | 4 +++-
 ld/lexsup.c | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/ld/ld.texi b/ld/ld.texi
index 9f562935be..25ce1d6524 100644
--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -1759,7 +1759,9 @@ Print a summary of all target-specific options on the standard output and exit.
 @kindex -Map=@var{mapfile}
 @item -Map=@var{mapfile}
 Print a link map to the file @var{mapfile}.  See the description of the
-@option{-M} option, above.
+@option{-M} option, above. Specifying the empty string as @var{mapfile}
+(that is, @code{-Map=}) causes the link map to be written to a file
+named after the @var{output} file, with @code{.map} appended.
 
 @cindex memory usage
 @kindex --no-keep-memory
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 2597e2d630..e7ab07b50f 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -1617,6 +1617,11 @@ parse_args (unsigned argc, char **argv)
 	}
     }
 
+  if (config.map_filename && !config.map_filename[0]) {
+    if (asprintf (&config.map_filename, "%s.map", output_filename) < 0)
+      einfo (_("%F%P: %s: can not create name of map file: %E\n"));
+  }
+
   if (command_line.soname && command_line.soname[0] == '\0')
     {
       einfo (_("%P: SONAME must not be empty string; ignored\n"));
-- 
2.23.0

Re: [PATCH] allow empty string as argument to -Map

[Nick Clifton]

Hi Rasmus,

> So, as the next best thing, use a previously bogus value (the empty
> string) as a sentinel to indicate "use the output filename + .map".

Thanks for suggesting this patch.  I have applied it along with a couple
of changes:

  * If a filename is supplied, but it references a directory, then
    create a file inside that directory, again based upon the output
    filename with .map appended.  (I actually did this to make it
    easier to test the new feature, but it strikes that it might be
    useful in its own right).

  * Added a test case.

  * Added an entry to the ld/NEWS file.

  * Updated the --help output from the linker to show that the
    filename is optional.

  * Minor formatting fixes, and the addition of a few comments.

  * Created a ChangeLog entry.

Cheers
  Nick

ld/ChangeLog
2020-05-27  Rasmus Villemoes <rv@rasmusvillemoes.dk>
	    Nick Clifton  <nickc@redhat.com>

	* lexsup.c (parse_args): If the map filename is defined but empty
	create a name based upon the output file name.  If the name is
	defined but refers to a directory create a file inside the
	directory based on the output file name.
	* ld.texi: Document the new feature.
	* testsuite/ld-script/map-address.exp: Add test of new feature.
	* NEWS: Mention the new feature.

Re: [PATCH] allow empty string as argument to -Map

[Rasmus Villemoes]

On 27/05/2020 18.50, Nick Clifton wrote:
> Hi Rasmus,
> 
>> So, as the next best thing, use a previously bogus value (the empty
>> string) as a sentinel to indicate "use the output filename + .map".
> 
> Thanks for suggesting this patch.  I have applied it along with a couple
> of changes:
> 
>   * If a filename is supplied, but it references a directory, then
>     create a file inside that directory, again based upon the output
>     filename with .map appended.  (I actually did this to make it
>     easier to test the new feature, but it strikes that it might be
>     useful in its own right).

That makes a lot of sense, thanks. And one can get "my" behaviour by
passing "-Map=." or "-Map .", which...

>   * Updated the --help output from the linker to show that the
>     filename is optional.

... eliminates the somewhat clumsy need to specify the empty string as
argument, either via -Map=  or actually have the shell insert an empty
string, -Map ''. I actually did attempt to modify the help text, but as
the argument isn't really optional, I couldn't find a way to indicate
that the empty string was meaningful - I worry a bit that the current

  -Map [FILE]         Write a map file (default: <outputname>.map)

can lead people to think that one can actually omit the argument - and

  -Map --gc-sections

would just put the map in the file --gc-sections, but otherwise work.

So before this gets set in stone, can I retract my suggestion of
assigning meaning to the empty string (maybe continue to handle it
internally equivalent to ".", but not advertising it)? For the help
text, it's hard to explain in one line, but perhaps something like

  -Map FILE/DIR     Write a map file to FILE or DIR/<outputname>.map

(And perhaps change "map file" to "link map" or "linker map")

Rasmus

Re: [PATCH] allow empty string as argument to -Map

[Fangrui Song]

On Wed, May 27, 2020 at 5:33 PM Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote:
>
> On 27/05/2020 18.50, Nick Clifton wrote:
> > Hi Rasmus,
> >
> >> So, as the next best thing, use a previously bogus value (the empty
> >> string) as a sentinel to indicate "use the output filename + .map".
> >
> > Thanks for suggesting this patch.  I have applied it along with a couple
> > of changes:
> >
> >   * If a filename is supplied, but it references a directory, then
> >     create a file inside that directory, again based upon the output
> >     filename with .map appended.  (I actually did this to make it
> >     easier to test the new feature, but it strikes that it might be
> >     useful in its own right).
>
> That makes a lot of sense, thanks. And one can get "my" behaviour by
> passing "-Map=." or "-Map .", which...
>
> >   * Updated the --help output from the linker to show that the
> >     filename is optional.
>
> ... eliminates the somewhat clumsy need to specify the empty string as
> argument, either via -Map=  or actually have the shell insert an empty
> string, -Map ''. I actually did attempt to modify the help text, but as
> the argument isn't really optional, I couldn't find a way to indicate
> that the empty string was meaningful - I worry a bit that the current
>
>   -Map [FILE]         Write a map file (default: <outputname>.map)
>
> can lead people to think that one can actually omit the argument - and
>
>   -Map --gc-sections
>
> would just put the map in the file --gc-sections, but otherwise work.
>
> So before this gets set in stone, can I retract my suggestion of
> assigning meaning to the empty string (maybe continue to handle it
> internally equivalent to ".", but not advertising it)? For the help
> text, it's hard to explain in one line, but perhaps something like
>
>   -Map FILE/DIR     Write a map file to FILE or DIR/<outputname>.map
>
> (And perhaps change "map file" to "link map" or "linker map")
>
> Rasmus
>

An empty argument value is very uncommon. Does an empty string work in
a response file?

Re: [PATCH] allow empty string as argument to -Map

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

Hi Rasmus,

> That makes a lot of sense, thanks. And one can get "my" behaviour by
> passing "-Map=." or "-Map .", 

Ha!  I had not even though of that.

> ... eliminates the somewhat clumsy need to specify the empty string as
> argument, 

Agreed.

> So before this gets set in stone, can I retract my suggestion of
> assigning meaning to the empty string (maybe continue to handle it
> internally equivalent to ".", but not advertising it)?

I have made the change.  See the attached patch.  I decided that for
the empty string case it would be best to generate a message telling
the user and ignoring the map file request.


> (And perhaps change "map file" to "link map" or "linker map")

Also done.

Cheers
  Nick

ld/ChangeLog
2020-05-28  Nick Clifton  <nickc@redhat.com>

	* lexsup.c (parse_args): Generate an error if a name is not
	provided to the -Map option.
	(ld_options): Mention that the -Map option supports a directory
	name as an argument.
	* NEWS: Remove mention of support for an empty string as an
	argument to -Map.
	* ld.texi: Likewise.

[-- Attachment #2: map.patch --]
[-- Type: text/x-patch, Size: 3906 bytes --]

diff --git a/ld/NEWS b/ld/NEWS
index 98f07a73e1..2240aeb788 100644
--- a/ld/NEWS
+++ b/ld/NEWS
@@ -1,9 +1,8 @@
 -*- text -*-
 
 * The -Map=<filename> command line option has been extended so that if
-  <filename> is omitted then a file called <output-filename>.map will be
-  created.  Plus if <filename> is a directory then
-  <filename>/<output-filename>.map will be created.
+  <filename> is a directory then <filename>/<output-filename>.map will be
+  created.
 
 * Add a command-line option for ELF linker, --warn-textrel, to warn that
   DT_TEXTREL is set in a position-independent executable or shared object.
diff --git a/ld/ld.texi b/ld/ld.texi
index 52342523ed..cb38f47cd3 100644
--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -1760,12 +1760,10 @@ Print a summary of all target-specific options on the standard output and exit.
 @kindex -Map=@var{mapfile}
 @item -Map=@var{mapfile}
 Print a link map to the file @var{mapfile}.  See the description of the
-@option{-M} option, above.  Specifying the empty string as @var{mapfile}
-(that is, @code{-Map=}) causes the link map to be written to a file
-named after the @var{output} file, with @code{.map} appended.
-Specifying a directory as @var{mapfile} causes the link map to be
-written into a file inside the directory.  The name of the file is
-again based upon the @var{output} filename with @code{.map} appended.
+@option{-M} option, above.  Specifying a directory as @var{mapfile}
+causes the linker map to be written into a file inside the directory.
+The name of the file is based upon the @var{output} filename with
+@code{.map} appended.
 
 @cindex memory usage
 @kindex --no-keep-memory
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 49c4f23950..781f58aff7 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -359,7 +359,7 @@ static const struct ld_option ld_options[] =
   { {"init", required_argument, NULL, OPTION_INIT},
     '\0', N_("SYMBOL"), N_("Call SYMBOL at load-time"), ONE_DASH },
   { {"Map", required_argument, NULL, OPTION_MAP},
-    '\0', N_("[FILE]"), N_("Write a map file (default: <outputname>.map)"), ONE_DASH },
+    '\0', N_("FILE/DIR"), N_("Write a linker map to FILE or DIR/<outputname>.map"), ONE_DASH },
   { {"no-define-common", no_argument, NULL, OPTION_NO_DEFINE_COMMON},
     '\0', NULL, N_("Do not define Common storage"), TWO_DASHES },
   { {"no-demangle", no_argument, NULL, OPTION_NO_DEMANGLE },
@@ -1598,29 +1598,33 @@ parse_args (unsigned argc, char **argv)
   /* Run a couple of checks on the map filename.  */
   if (config.map_filename)
     {
-      /* If name has been provided then use the
-	 output filename with a .map extension.  */
       if (config.map_filename[0] == 0)
 	{
-	  /* FIXME: This is a memory leak as the string is never freed.  */
-	  if (asprintf (&config.map_filename, "%s.map", output_filename) < 0)
-	    einfo (_("%F%P: %s: can not create name of map file: %E\n"));
+	  einfo (_("%P: no file/directory name provided for map output; ignored\n"));
+	  config.map_filename = NULL;
 	}
       else
 	{
 	  struct stat s;
 
 	  /* If the map filename is actually a directory then create
-	     a file inside it, again based upon the output filename.  */
+	     a file inside it, based upon the output filename.  */
 	  if (stat (config.map_filename, &s) >= 0
 	      && S_ISDIR (s.st_mode))
 	    {
 	      char * new_name;
 
-	      /* FIXME: Another memory leak.  */
+	      /* FIXME: This is a (trivial) memory leak.  */
 	      if (asprintf (&new_name, "%s/%s.map",
 			    config.map_filename, output_filename) < 0)
-		einfo (_("%F%P: %s: can not create name of map file: %E\n"));
+		{
+		  /* If this alloc fails then something is probably very
+		     wrong.  Better to halt now rather than continue on
+		     into more problems.  */
+		  einfo (_("%P%F: cannot create name for linker map file: %E\n"));
+		  new_name = NULL;
+		}
+
 	      config.map_filename = new_name;
 	    }
 	}

Re: [PATCH] allow empty string as argument to -Map

[Rasmus Villemoes]

On 28/05/2020 11.54, Nick Clifton wrote:
> Hi Rasmus,
> 
>> That makes a lot of sense, thanks. And one can get "my" behaviour by
>> passing "-Map=." or "-Map .", 
> 
> Ha!  I had not even though of that.
> 
>> ... eliminates the somewhat clumsy need to specify the empty string as
>> argument, 
> 
> Agreed.

Sorry for resurrecting this, and not thinking of this sooner, but it
just occurred to me that passing -Map=. only works automatically if the
-o argument is a relative path. If the -o argument is absolute, that
will lead to trying to create a map file at, say,
".//home/ravi/some/project/foo.so.out", which won't work given that the
working directory is very likely already "/home/ravi/some/project". One
could pass -Map=/, but that would then break in the other direction when
the -o argument is relative.

Since the main point of this is to be able to just do "LDFLAGS +=
<something>" to always get a linker map for each output artifact,
without hooking into each individual project's build system and figuring
out how to add that flag to each build target, can we special-case . to
always mean "generate a map file next to the output file", i.e. in that
case check whether output_filename starts with '/' and if so do not
prepend "./" (but still append ".map", of course)? Or is there some
better way?

Rasmus

Re: [PATCH] allow empty string as argument to -Map

[Nick Clifton]

Hi Rasmus,

> Sorry for resurrecting this, and not thinking of this sooner, but it
> just occurred to me that passing -Map=. only works automatically if the
> -o argument is a relative path. If the -o argument is absolute, that
> will lead to trying to create a map file at, say,
> ".//home/ravi/some/project/foo.so.out", which won't work given that the
> working directory is very likely already "/home/ravi/some/project". One
> could pass -Map=/, but that would then break in the other direction when
> the -o argument is relative.

> Since the main point of this is to be able to just do "LDFLAGS +=
> <something>" to always get a linker map for each output artifact,
> without hooking into each individual project's build system and figuring
> out how to add that flag to each build target, can we special-case . to
> always mean "generate a map file next to the output file", i.e. in that
> case check whether output_filename starts with '/' and if so do not
> prepend "./" (but still append ".map", of course)? Or is there some
> better way?

Hmm, I think that that would be confusing.  Suppose for example that
the link involved "-Map=.. -o /foo/bar".  Where would the user expect
the map file to appear ?  In ../foo/bar.map or ../bar.map or /foo/bar.map ?

My feeling is that we ought to extract the basename of any output file
and use that, and always honour the directory name specified in the -Map
option.  ie:

  -Map=. -o foo       => creates ./foo.map
  -Map=. -o /foo      => creates ./foo.map 
  -Map=. -o /foo/bar  => creates ./bar.map
  -Map=/ -o foo       => creates /foo.map
  -Map=/ -o /foo/bar  => creates /bar.map
  -Map=/foo -o bar    => creates /foo/bar.map
  -Map=../ -o foo/bar => creates ../bar.map.  (NB/ does not create ../foo/bar.map)

Thoughts ?

Cheers
  Nick

Re: [PATCH] allow empty string as argument to -Map

[Rasmus Villemoes]

On 29/10/2020 16.52, Nick Clifton wrote:
> Hi Rasmus,
> 
>> Sorry for resurrecting this, and not thinking of this sooner, but it
>> just occurred to me that passing -Map=. only works automatically if the
>> -o argument is a relative path. If the -o argument is absolute, that
>> will lead to trying to create a map file at, say,
>> ".//home/ravi/some/project/foo.so.out", which won't work given that the
>> working directory is very likely already "/home/ravi/some/project". One
>> could pass -Map=/, but that would then break in the other direction when
>> the -o argument is relative.
> 
>> Since the main point of this is to be able to just do "LDFLAGS +=
>> <something>" to always get a linker map for each output artifact,
>> without hooking into each individual project's build system and figuring
>> out how to add that flag to each build target, can we special-case . to
>> always mean "generate a map file next to the output file", i.e. in that
>> case check whether output_filename starts with '/' and if so do not
>> prepend "./" (but still append ".map", of course)? Or is there some
>> better way?
> 
> Hmm, I think that that would be confusing.  Suppose for example that
> the link involved "-Map=.. -o /foo/bar".  Where would the user expect
> the map file to appear ?  In ../foo/bar.map or ../bar.map or /foo/bar.map ?
> 
> My feeling is that we ought to extract the basename of any output file
> and use that, and always honour the directory name specified in the -Map
> option.  ie:
> 
>   -Map=. -o foo       => creates ./foo.map
>   -Map=. -o /foo      => creates ./foo.map 
>   -Map=. -o /foo/bar  => creates ./bar.map
>   -Map=/ -o foo       => creates /foo.map
>   -Map=/ -o /foo/bar  => creates /bar.map
>   -Map=/foo -o bar    => creates /foo/bar.map
>   -Map=../ -o foo/bar => creates ../bar.map.  (NB/ does not create ../foo/bar.map)

Yes, that's another problem with the current logic, even for relative -o
arguments: If one specifies -Map=maps/, it may be that maps/ exists, but
for -o foo/bar, we'd try to create maps/foo/bar.map - the Makefile (or
whatnot) obviously ensures foo/ exists, but maps/foo/ would not
automatically be created.

However, the problem with using (only) the basename of the -o argument
is that the build (not the single ld invocation, but the surrounding
build system) could produce several binaries of the same name in
different directories, so we (a) would clobber the .map files and (b)
cannot tell which binary the single surviving .map belongs to.

So I don't think there's any way around having _some_ magic value mean
'take the -o argument, append ".map"'. That magic value can be almost
anything

-Map=%
-Map=/ # nobody would put stuff in the root directory
-Map=AUTO

(and if anybody really wants a map file called AUTO, they could say
-Map=./AUTO).

> Thoughts ?

A whole other option, which may be even easier to hook into arbitrary
build systems (not everybody honours/forwards LDFLAGS), is to say "If no
-Map argument is given, but the env variable LDAUTOMAP is set, use the
-o argument with .map appended". ld does already look at a few
environment variables that can be overridden with options, so it's not
completely unprecedented.

Rasmus

Re: [PATCH] allow empty string as argument to -Map

[Nick Clifton]

Hi Rasmus,

>> My feeling is that we ought to extract the basename of any output file
>> and use that, and always honour the directory name specified in the -Map
>> option. 

> However, the problem with using (only) the basename of the -o argument
> is that the build (not the single ld invocation, but the surrounding
> build system) could produce several binaries of the same name in
> different directories, so we (a) would clobber the .map files and (b)
> cannot tell which binary the single surviving .map belongs to.

That does not sound likely to me.  In order for a clobber to occur
the user would have to build multiple binaries with the same output
name and using the same map directory.  But in order for the output
binaries not to clobber each other they would have to be built into
different output directories.  ie:

  -o foo/bar -Map=/mapdir
  -o baz/bar -Map=/mapdir

It seems to me that this is only likely to occur when creating multiple
versions of the same binary (eg with different ABI options) and the map
dir is being automatically added to the build.  (If the user added the
-Map option then they would almost certainly include the discriminating 
directory name as well).


> -Map=AUTO

Hmm. maybe.

Another possibility is to have the linker not overwrite an existing
map file, but instead append a timestamp/random number instead.  So

  -o foo -Map=.

would create ./foo.map if it does not exist, or .foo.202010301630.map
if foo.map exists.  As distinguishing between these map files, I would
assume that looking at their contents, and seeing the input files mentioned
should be enough.


> -Map argument is given, but the env variable LDAUTOMAP is set,

I am strongly against environment variables, if they can be avoided.
I know that they linker does use some, but I consider this to be a
mistake.  The bug problem with environment variables is that they make
generating bug reports really hard.  Most users never include
environment variables in their reports, and often they do not even
know that they are there.

Cheers
  Nick

Re: [PATCH] allow empty string as argument to -Map

[Rasmus Villemoes]

On 30/10/2020 17.35, Nick Clifton wrote:
> Hi Rasmus,
> 
>>> My feeling is that we ought to extract the basename of any output file
>>> and use that, and always honour the directory name specified in the -Map
>>> option. 
> 
>> However, the problem with using (only) the basename of the -o argument
>> is that the build (not the single ld invocation, but the surrounding
>> build system) could produce several binaries of the same name in
>> different directories, so we (a) would clobber the .map files and (b)
>> cannot tell which binary the single surviving .map belongs to.
> 
> That does not sound likely to me.  In order for a clobber to occur
> the user would have to build multiple binaries with the same output
> name and using the same map directory.  But in order for the output
> binaries not to clobber each other they would have to be built into
> different output directories.  ie:
> 
>   -o foo/bar -Map=/mapdir
>   -o baz/bar -Map=/mapdir
> 
> It seems to me that this is only likely to occur when creating multiple
> versions of the same binary (eg with different ABI options) and the map
> dir is being automatically added to the build.  (If the user added the
> -Map option then they would almost certainly include the discriminating 
> directory name as well).

Yes, exactly, please see my original use case: I'd like some way to get
the build to produce map files for all the binaries/solibs etc.
generated, without patching each individual project's build system and
figuring out where to hook in.

This is in the context of building an embedded system using Yocto. So
apart from the job of looking at each piece of software (systemd,
util-linux, busybox, glibc, ....) and understand how that gets built, I
also have to create .patch files, and temporarily add them to the
individual recipes. Most build systems do honour LDFLAGS set from
outside, so I'd like to do that LDFLAGS += "-Map=something" globally.
But also why an environment variable understood by ld itself would be
even more convenient, as that wouldn't even rely on the build system
passing on inherited LDFLAGS.

> 
>> -Map=AUTO
> 
> Hmm. maybe.
> 
> Another possibility is to have the linker not overwrite an existing
> map file, but instead append a timestamp/random number instead.  So
> 
>   -o foo -Map=.
> 
> would create ./foo.map if it does not exist, or .foo.202010301630.map
> if foo.map exists.  As distinguishing between these map files, I would
> assume that looking at their contents, and seeing the input files mentioned
> should be enough.

I suppose you're right about the chance of different binaries in
different directories having the same name is quite low, and that, in
case of a clash, one could distinguish based on contents. But then the
problem is that when one rebuilds without cleaning the destination
directory, we're actually guaranteed to hit that clash, even if no
binaries clash with each other. I think it would add too much complexity
and potential for confusion. And actually, sometimes passing "-Map=."
wouldn't work; the build system could have a read-only source directory
be CWD and have all output go to some other directory.

[*] So I still think that having _some way_ to ask ld to create a map
file named "<-o argument>.map" is the most convenient; no issues with
the target directory possibly not existing or not being writable; no
issues with name clashes; the .map gets automatically
truncated/overwritten when the binary gets rebuilt.

> 
>> -Map argument is given, but the env variable LDAUTOMAP is set,
> 
> I am strongly against environment variables, if they can be avoided.
> I know that they linker does use some, but I consider this to be a
> mistake.  The bug problem with environment variables is that they make
> generating bug reports really hard.  Most users never include
> environment variables in their reports, and often they do not even
> know that they are there.

So I agree with you in general. But I'd also argue that for the case of
Map files, which are mostly/only for diagnostics, a developer setting
LDAUTOMAP would know what he's doing. But if I can have some way of
doing [*] via the command line, that would also work for the majority of
cases. And instead of a magic -Map= value, it could also simply be a new
--automap option (with an explicit -Map= taking precedence).

Rasmus

Re: [PATCH] allow empty string as argument to -Map

[Nick Clifton]

Hi Rasmus,

> Yes, exactly, please see my original use case: I'd like some way to get
> the build to produce map files for all the binaries/solibs etc.
> generated, without patching each individual project's build system and
> figuring out where to hook in.

OK, so how about this:

  -Map=<file>   Put the map into <file>.  <file> can be relative or absolute.
  -Map=<dir>    Put the map into <dir>/basename (outfile).map.  Overwrites existing maps.
  -Map=%        Put the map into (outfile).map.  Includes all path components in (outfile)

Would this work for you ?

Cheers
  Nick

Re: [PATCH] allow empty string as argument to -Map

[Rasmus Villemoes]

On 04/11/2020 12.07, Nick Clifton wrote:
> Hi Rasmus,
> 
>> Yes, exactly, please see my original use case: I'd like some way to get
>> the build to produce map files for all the binaries/solibs etc.
>> generated, without patching each individual project's build system and
>> figuring out where to hook in.
> 
> OK, so how about this:
> 
>   -Map=<file>   Put the map into <file>.  <file> can be relative or absolute.
>   -Map=<dir>    Put the map into <dir>/basename (outfile).map.  Overwrites existing maps.
>   -Map=%        Put the map into (outfile).map.  Includes all path components in (outfile)
> 
> Would this work for you ?
> 

Yes, absolutely. Thanks.

Rasmus

Re: [PATCH] allow empty string as argument to -Map

[Fangrui Song]

On 2020-11-04, Rasmus Villemoes wrote:
>On 04/11/2020 12.07, Nick Clifton wrote:
>> Hi Rasmus,
>>
>>> Yes, exactly, please see my original use case: I'd like some way to get
>>> the build to produce map files for all the binaries/solibs etc.
>>> generated, without patching each individual project's build system and
>>> figuring out where to hook in.
>>
>> OK, so how about this:
>>
>>   -Map=<file>   Put the map into <file>.  <file> can be relative or absolute.
>>   -Map=<dir>    Put the map into <dir>/basename (outfile).map.  Overwrites existing maps.

This looks like a complex operation. Is it really needed?

>>   -Map=%        Put the map into (outfile).map.  Includes all path components in (outfile)

Maybe -Map=%.map to avoid hard coding the extension name?

>> Would this work for you ?
>>
>
>Yes, absolutely. Thanks.
>
>Rasmus

Re: [PATCH] allow empty string as argument to -Map

[Nick Clifton]

Hi Fangrui,

>>   -Map=<file>   Put the map into <file>.  <file> can be relative or absolute.
>>   -Map=<dir>    Put the map into <dir>/basename (outfile).map.  Overwrites existing maps.

> This looks like a complex operation. Is it really needed?

This actually reflects the current situation (well without the basename operation).
Providing a directory name for the map option is useful as it allows build systems to use the option without having to worry about generating unique map names.


>>   -Map=%        Put the map into (outfile).map.  Includes all path components in (outfile)
> 
> Maybe -Map=%.map to avoid hard coding the extension name?

Except that we already hard code the .map extension when the directory version of the option is used.  But it would be simple enough to extend the specification so that:

   -Map=%         Puts the map into (outfile).map
   -Map=%.<foo>   Puts the map into (outfile).<foo>

Cheers
   Nick

Re: [PATCH] allow empty string as argument to -Map

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 162 bytes --]

Hi Fangrui, Hi Rasmus,

   Any comments on the attached patch ?

   It implements the basename and % character changes as previously discussed.

Cheers
   Nick



[-- Attachment #2: ld-map-directory.patch --]
[-- Type: text/x-patch, Size: 6924 bytes --]

diff --git a/ld/ld.texi b/ld/ld.texi
index cf8f05c39e..9b74b893c1 100644
--- a/ld/ld.texi
+++ b/ld/ld.texi
@@ -1866,10 +1866,34 @@ Print a summary of all target-specific options on the standard output and exit.
 @kindex -Map=@var{mapfile}
 @item -Map=@var{mapfile}
 Print a link map to the file @var{mapfile}.  See the description of the
-@option{-M} option, above.  Specifying a directory as @var{mapfile}
-causes the linker map to be written into a file inside the directory.
-The name of the file is based upon the @var{output} filename with
-@code{.map} appended.
+@option{-M} option, above.  If @var{mapfile} is just the character
+@code{-} then the map will be written to stdout.
+
+Specifying a directory as @var{mapfile} causes the linker map to be
+written as a file inside the directory.  Normally name of the file
+inside the directory is computed as the basename of the @var{output}
+file with @code{.map} appended.   If however the special character
+@code{%} is used then this will be replaced by the full path of the
+output file.  Additionally if there are any characters after the
+@var{%} symbol then @code{.map} will no longer be appended.
+
+@smallexample
+ -o foo.exe -Map=bar                  [Creates ./bar]
+ -o ../dir/foo.exe -Map=bar           [Creates ./bar]
+ -o foo.exe -Map=../dir               [Creates ../dir/foo.exe.map]
+ -o ../dir2/foo.exe -Map=../dir       [Creates ../dir/foo.exe.map]
+ -o foo.exe -Map=%                    [Creates ./foo.exe.map]
+ -o ../dir/foo.exe -Map=%             [Creates ../dir/foo.exe.map]
+ -o foo.exe -Map=%.bar                [Creates ./foo.exe.bar]
+ -o ../dir/foo.exe -Map=%.bar         [Creates ../dir/foo.exe.bar]
+ -o ../dir2/foo.exe -Map=../dir/%     [Creates ../dir/../dir2/foo.exe.map]
+ -o ../dir2/foo.exe -Map=../dir/%.bar [Creates ../dir/../dir2/foo.exe.bar]
+@end smallexample
+
+It is an error to specify more than one @code{%} character.
+
+If the map file already exists then it will be overwritten by this
+operation.
 
 @cindex memory usage
 @kindex --no-keep-memory
diff --git a/ld/lexsup.c b/ld/lexsup.c
index eae64932df..45c29d82ff 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -22,8 +22,10 @@
 #include "bfd.h"
 #include "bfdver.h"
 #include "libiberty.h"
+#include "filenames.h"
 #include <stdio.h>
 #include <string.h>
+#include <errno.h>
 #include "safe-ctype.h"
 #include "getopt.h"
 #include "bfdlink.h"
@@ -1700,35 +1702,84 @@ parse_args (unsigned argc, char **argv)
   /* Run a couple of checks on the map filename.  */
   if (config.map_filename)
     {
+      char * new_name = NULL;
+      char * percent;
+      int    res = 0;
+
       if (config.map_filename[0] == 0)
 	{
 	  einfo (_("%P: no file/directory name provided for map output; ignored\n"));
 	  config.map_filename = NULL;
 	}
+      else if (strcmp (config.map_filename, "-") == 0)
+	; /* Write to stdout.  Handled in main().  */
+      else if ((percent = strchr (config.map_filename, '%')) != NULL)
+	{
+	  /* FIXME: Check for a second % character and issue an error ?  */
+
+	  /* Construct a map file by replacing the % character with the (full)
+	     output filename.  If the % character was the last character in
+	     the original map filename then add a .map extension.  */
+	  if (percent == config.map_filename)
+	    {
+	      if (percent[1] == 0)
+		res = asprintf (&new_name, "%s.map", output_filename);
+	      else
+		res = asprintf (&new_name, "%s%s",
+				output_filename, percent + 1);
+	    }
+	  else
+	    {
+	      percent[0] = 0;
+	      if (percent[1] == 0)
+		res = asprintf (&new_name, "%s%s.map",
+				config.map_filename, output_filename);
+	      else
+		res = asprintf (&new_name, "%s%s%s",
+				config.map_filename, output_filename,
+				percent + 1);
+	    }
+
+	  /* FIXME: Should we ensure that any directory components in new_name exist ?  */
+	}
       else
 	{
 	  struct stat s;
 
 	  /* If the map filename is actually a directory then create
 	     a file inside it, based upon the output filename.  */
-	  if (stat (config.map_filename, &s) >= 0
-	      && S_ISDIR (s.st_mode))
+	  if (stat (config.map_filename, &s) < 0)
 	    {
-	      char * new_name;
-
-	      /* FIXME: This is a (trivial) memory leak.  */
-	      if (asprintf (&new_name, "%s/%s.map",
-			    config.map_filename, output_filename) < 0)
-		{
-		  /* If this alloc fails then something is probably very
-		     wrong.  Better to halt now rather than continue on
-		     into more problems.  */
-		  einfo (_("%P%F: cannot create name for linker map file: %E\n"));
-		  new_name = NULL;
-		}
-
-	      config.map_filename = new_name;
+	      if (errno != ENOENT)
+		einfo (_("%P: cannot stat linker map file: %E\n"));
+	    }
+	  else if (S_ISDIR (s.st_mode))
+	    {
+	      char lastc = config.map_filename[strlen (config.map_filename) - 1];
+	      res = asprintf (&new_name, "%s%s%s.map",
+			      config.map_filename,
+			      IS_DIR_SEPARATOR (lastc) ? "" : "/",
+			      lbasename (output_filename));
+	    }
+	  else if (! S_ISREG (s.st_mode))
+	    {
+	      einfo (_("%P: linker map file is not a regular file\n"));
+	      config.map_filename = NULL;
 	    }
+	  /* else FIXME: Check write permission ?  */
+	}
+
+      if (res < 0)
+	{
+	  /* If the asprintf failed then something is probably very
+	     wrong.  Better to halt now rather than continue on
+	     into more problems.  */
+	  einfo (_("%P%F: cannot create name for linker map file: %E\n"));
+	}
+      else if (new_name != NULL)
+	{
+	  /* This is a trivial memory leak.  */
+	  config.map_filename = new_name;
 	}
     }
 
diff --git a/ld/testsuite/ld-scripts/map-address.exp b/ld/testsuite/ld-scripts/map-address.exp
index 1f9457a8cd..e8d3683c01 100644
--- a/ld/testsuite/ld-scripts/map-address.exp
+++ b/ld/testsuite/ld-scripts/map-address.exp
@@ -46,6 +46,7 @@ if {[regexp_diff \
     pass $testname
 }
 
+
 set testname "map to directory"
 
 if {![ld_link $ld tmpdir/map-address \
@@ -67,3 +68,49 @@ if {[regexp_diff \
 } else {
     pass $testname
 }
+
+
+set testname "map to % directory"
+
+if {![ld_link $ld tmpdir/map-address \
+	 "$LDFLAGS -T $srcdir/$subdir/map-address.t \
+	  tmpdir/map-address.o \
+	  -Map=tmpdir/% --output fred"]} {
+    fail $testname
+    return
+}
+
+if [is_remote host] then {
+    remote_upload host "tmpdir/fred.map"
+}
+
+if {[regexp_diff \
+	       "tmpdir/fred.map" \
+	       "$srcdir/$subdir/map-address.d"]} {
+    fail $testname
+} else {
+    pass $testname
+}
+
+
+set testname "map to %.foo directory"
+
+if {![ld_link $ld tmpdir/map-address \
+	 "$LDFLAGS -T $srcdir/$subdir/map-address.t \
+	  tmpdir/map-address.o \
+	  -Map=tmpdir/%.foo --output fred"]} {
+    fail $testname
+    return
+}
+
+if [is_remote host] then {
+    remote_upload host "tmpdir/fred.foo"
+}
+
+if {[regexp_diff \
+	       "tmpdir/fred.foo" \
+	       "$srcdir/$subdir/map-address.d"]} {
+    fail $testname
+} else {
+    pass $testname
+}

Re: [PATCH] allow empty string as argument to -Map

[Rasmus Villemoes]

On 05/11/2020 12.41, Nick Clifton wrote:
> Hi Fangrui, Hi Rasmus,
> 
>   Any comments on the attached patch ?
> 
>   It implements the basename and % character changes as previously
> discussed.

LGTM. I think you can simplify it somewhat: Always overwrite the % with
a nul byte and construct the path using the part of the -Map argument
before the % (which will be empty in the case where % is the first
character).

So instead of the four different asprintf, I think you can just do

  percent[0] = 0;
  res = asprintf(&new_name, "%s%s%s", config.map_filename,
output_filename, percent[1] ? percent + 1 : ".map");

no?

Thanks for working on this.

Rasmus

Re: [PATCH] allow empty string as argument to -Map

[Nick Clifton]

Hi Rasmus,

> So instead of the four different asprintf, I think you can just do
> 
>    percent[0] = 0;
>    res = asprintf(&new_name, "%s%s%s", config.map_filename,
> output_filename, percent[1] ? percent + 1 : ".map");
> 
> no?

Good point - I have applied the patch with this change made.

Cheers
   Nick