1041962 - build to fail with musl libc with: error: 'basename' was not declared in this scope

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Natanael Copa]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140616063102

Steps to reproduce:

tried to build firefox-31.0 on alpine linux, which uses musl libc


Actual results:

build failed with:
/home/ncopa/aports/main/xulrunner/src/mozilla-release/tools/profiler/LulElf.cpp:584:36: error: 'basename' was not declared in this scope
   string base = basename(c_filename);
                                    ^



Expected results:

build should have completed.

Comment 1

[Natanael Copa]

The problem is that there are 2 different implementations of basename(3) on linux, one posix version and one GNU version. To get the posix version you should #include <libgen.h> and to get GNU version you should #define _GNU_SOURCE. see http://linux.die.net/man/3/basename

Since none of those are done, the build fails on musl libc which is very strict on those things.

Looking at the code comments, it looks like posix behavior is expected (extra malloc/copy/free is done to avoid original string be modified).

Possible options:
* add #define _GNU_SOURCE to get GNU version (what happens in practice now on glibc)
* add #include <libgen.h> to get posix version (likely more portable)
* avoid basename alltogehter and instead do something like:
    // basename(3) might modify its argument. We avoid extra malloc/copy/free
    char *p = strrchr(filename.c_str(), '/');
    string base = p ? p : filename.c_str();

I can send a patch if you tell me what you prefer.

Comment 2

[Natanael Copa]

Correction, it appears that _GNU_SOURCE is actually set so I don't know what is going on really.

Comment 3

[Natanael Copa]

Something like this should work around it and provide somewhat faster code as it avoids extra malloc, copy and free:

--- mozilla-release.orig/tools/profiler/LulElf.cpp
+++ mozilla-release/tools/profiler/LulElf.cpp
@@ -579,10 +579,10 @@
 // Return the non-directory portion of FILENAME: the portion after the
 // last slash, or the whole filename if there are no slashes.
 string BaseFileName(const string &filename) {
-  // Lots of copies!  basename's behavior is less than ideal.
-  char *c_filename = strdup(filename.c_str());
-  string base = basename(c_filename);
-  free(c_filename);
+  // basename's behavior is less than ideal so avoid it
+  const char *c_filename = filename.c_str();
+  const char *p = strrchr(c_filename, '/');
+  string base = p ? p+1 : c_filename;
   return base;
 }

Comment 4

[Natanael Copa]

(In reply to Natanael Copa from comment #2)
> Correction, it appears that _GNU_SOURCE is actually set so I don't know what
> is going on really.

<nsz> ncopa: the gnu basename in string.h is only visible in c mode, not c++
<nsz> because the gnu one has no prototype
<nsz> (so it accepts const char * arg, not just char *)
<nsz> (but that does not work in c++)

So basically, the options we have are to either use posix implementaion (#include <libgen.h>) or avoid basename in c++ like in my patch above.

Comment 5

[:Gijs (he/him)]

Benoit, can you look into this? Seems like there's a patch and everything... :-)

Comment 6

[Benoit Girard (:BenWa)]

Isn't the point of basename that it deals with more complicated cases like escape sequences?

Comment 7

[Natanael Copa]

(In reply to Benoit Girard (:BenWa) from comment #6)
> Isn't the point of basename that it deals with more complicated cases like
> escape sequences?

Do you have any reference for that, or any examples of complicated cases with escape sequences? I cannot see anything in POSIX specification that indicates that handling escape sequences is required.

http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.html
(note that posix says that the original string might be modified though)

There is nothing in the glibc implementations that indicate that they handle escape sequences:
https://sourceware.org/git/?p=glibc.git;a=blob;f=string/basename.c
(note that GNU variant is similar to my proposed patch)

https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/xpg_basename.c

Neither does freebsd implementation:
http://svnweb.freebsd.org/base/head/lib/libc/gen/basename.c?view=markup

Or musl libc implementation:
http://git.musl-libc.org/cgit/musl/tree/src/misc/basename.c

The special cases POSIX says it should handle is when the string is NULL, zero length a single or trailing "/".

You can still simply add #include <libgen.h> if you prefer that. The current code with extra strdup/free will handle implementations that modifies the source string.

Comment 8

[maillist-mozilla]

i can confirm this issue trying to build firefox 31.0

/src/build/firefox/mozilla-release/tools/profiler/LulElf.cpp: In function 'std::string {anonymous}::BaseFileName(const string&)':
/src/build/firefox/mozilla-release/tools/profiler/LulElf.cpp:584:36: error: 'basename' was not declared in this scope
make[5]: *** [LulElf.o] Error 1

Comment 9

[Benoit Girard (:BenWa)]

Sweet, thanks for checking.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 11

[Julian Seward [:jseward]]

Comment on attachment 8516179 [details] [diff] [review]
patch

Review of attachment 8516179 [details] [diff] [review]:
-----------------------------------------------------------------

Duh, sorry about that.  I meant to select r+ but failed.  r+ really.

Comment 12

[Felix Janda]

I can reproduce the bug on current trunk. toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc has the same problem

Comment 13

[Charles Collicutt]

(In reply to Felix Janda from comment #12)
> I can reproduce the bug on current trunk.
> toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc has
> the same problem

According to the comments in LulElf.cpp, that code was derived from toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc so it makes sense that the original code still has this problem.

There's a patch to fix it upstream here: https://sourceforge.net/p/google-breakpad/tickets/631/

Comment 14

[Charles Collicutt]

The patch attached to this bug is now out-of-date: it's fine except that the affected file, LulElf.cpp, has moved since this patch was made. It is now in a 'lul' subdirectory.

Since this is still necessary for building against a non-glibc libc on Linux, would it be possible to create an up-to-date patch?

Comment 15

[Charles Collicutt]

I'm happy to create an up-to-date patch myself but don't want to claim credit for someone else's work (since all I'd do to create it would be to apply the existing patch to the right place in the source tree). Could you update your patch? Or would it be OK for me to do it? Thanks.

Comment 16

[Benoit Girard (:BenWa)]

Feel free to rebase it. I'm not fussy about the author field for such a small patch. Thanks for helping out!

Comment 17

[Charles Collicutt]

Attachment: basename.patch

Rebase the previous patch for this bug. Please could someone push this to the Try server?

Comment 18

[Charles Collicutt]

Sorry to bug you but please could you push this to the Try server for me?

Comment 19

[Benoit Girard (:BenWa)]

Sorry for the delay:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a660c20fea

Comment 20

[Julian Seward [:jseward]]

Charles, I am sorry for the excessive delay on this.  Do you
still wish to proceed with it?

Comment 21

[Charles Collicutt]

Yes, please do.

Comment 22

[Jory A. Pratt [:anarchy]]

(In reply to Julian Seward [:jseward] from comment #20)
> Charles, I am sorry for the excessive delay on this.  Do you
> still wish to proceed with it?

What is the hold up on getting this landed?

Comment 23

[:Gijs (he/him)]

(In reply to Jory A. Pratt from comment #22)
> (In reply to Julian Seward [:jseward] from comment #20)
> > Charles, I am sorry for the excessive delay on this.  Do you
> > still wish to proceed with it?
> 
> What is the hold up on getting this landed?

Comment 24

[Julian Seward [:jseward]]

(In reply to :Gijs from comment #23)
> > What is the hold up on getting this landed?

Basically, the difficulty in figuring out whether the replacement is
really functionally equivalent to the original:

* does it behave the same for all inputs?

* does it have the same leak/non-leak behaviour?

Some informal explanation of why the change is OK would be helpful.

Comment 25

[Natanael Copa]

(In reply to Julian Seward [:jseward] from comment #24)
> (In reply to :Gijs from comment #23)
> > > What is the hold up on getting this landed?
> 
> Basically, the difficulty in figuring out whether the replacement is
> really functionally equivalent to the original:
> 
> * does it behave the same for all inputs?
> 
> * does it have the same leak/non-leak behaviour?
> 
> Some informal explanation of why the change is OK would be helpful.

All the needed info is available in comment #1 and comment #7.

As I explained in comment #1, on linux GNU libc, you currently have GNU basename() behavior (due to the lack of #include<libgen.h>)

As explained in comment #7, the GNU basename() is very similar to my patch.

GNU basename():
````
  21 char *
  22 __basename (const char *filename)
  23 {
  24   char *p = strrchr (filename, '/');
  25   return p ? p + 1 : (char *) filename;
  26 }
````

My patch:
````
 string BaseFileName(const string &filename) {
-  // Lots of copies!  basename's behavior is less than ideal.
-  char *c_filename = strdup(filename.c_str());
-  string base = basename(c_filename);
-  free(c_filename);
+  // basename's behavior is less than ideal so avoid it
+  const char *c_filename = filename.c_str();
+  const char *p = strrchr(c_filename, '/');
+  string base = p ? p + 1 : c_filename;
   return base;
 }
````

Note in particular:

  24   char *p = strrchr (filename, '/');
  25   return p ? p + 1 : (char *) filename;

vs

+  const char *p = strrchr(c_filename, '/');
+  string base = p ? p + 1 : c_filename;


As you see the implementation is identical. I expect that behave the same for all inputs and have identical non-leak behavior.

Comment 26

[Jory A. Pratt [:anarchy]]

Attachment: 6001_add_missing_header_for_basename.patch

This is the least intrusive patch and ensure it continues to work with all libc variants.

Comment 27

[Natanael Copa]

(In reply to Jory A. Pratt from comment #26)
> Created attachment 8900998 [details] [diff] [review]
> 6001_add_missing_header_for_basename.patch
> 
> This is the least intrusive patch and ensure it continues to work with all
> libc variants.

Be aware that it modifies the behaviour on glibc systems when string ends with a slash "/".

Comment 28

[Jory A. Pratt [:anarchy]]

https://dxr.mozilla.org/mozilla-central/source/mozglue/baseprofiler/lul/LulElf.cpp#54

As you can see we are including libgen.h so there is no reason we should not include the header. These 2 profilers should be sync'd from what I understand.

Comment 29

[mforney]

Attachment: basename-string.patch

I'd really like to see this issue fixed, especially since it has such an easy fix. Right now every musl-based Linux distribution has to patch around this.

I attached another alternative patch, which just uses std::string functions to get the exact same behavior as the current GNU basename version (since we are already working with a std::string here).

Comment 30

[mforney]

Attachment: Bug 1041962 - Include libgen.h for basename

Currently, the GNU version of basename from string.h is used, which
has behavior that conflicts with the POSIX version in libgen.h.

The GNU basename is not available in all libcs. In particular, it
is missing in musl libc, causing a build failure:

error: 'basename' was not declared in this scope

The GNU version has the following implementation:

char *p = strrchr (filename, '/');
return p ? p + 1 : (char *) filename;

The POSIX version has slightly different semantics. It may modify
its argument string, or copy part of it to static storage. However,
it will also delete trailing slashes, while the GNU version will
return the empty string if there is a trailing slash.

This change resolves the issue by including libgen.h, adopting POSIX
basename semantics. This should be a safe change for the following
reasons:

• The google-breakpad code, from which this code was derived, has
  also switched to the POSIX basename:
  https://chromium.googlesource.com/breakpad/breakpad/+/072f86ca83bb7138fe33f10b6380badd9ef7f065%5E%21/#F4

• The version of LulElf.cpp in mozglue/baseprofiler has also switched
  to the POSIX basename:
  https://hg.mozilla.org/mozilla-central/annotate/de1c3ae8df14cdb2c94a817b02dcffcb2cee12e2/mozglue/baseprofiler/lul/LulElf.cpp#l54

• The BaseFileName function is called only with paths to ELF files,
  never directories, so the paths will never contain a trailing
  slash, and the two versions of basename will behave identically.

Comment 31

[Jory A. Pratt [:anarchy]]

(In reply to mforney from comment #30)

Created attachment 9123019 [details]
Bug 1041962 - Include libgen.h for basename

Currently, the GNU version of basename from string.h is used, which
has behavior that conflicts with the POSIX version in libgen.h.

The GNU basename is not available in all libcs. In particular, it
is missing in musl libc, causing a build failure:

error: 'basename' was not declared in this scope

The GNU version has the following implementation:

char *p = strrchr (filename, '/');
return p ? p + 1 : (char *) filename;

The POSIX version has slightly different semantics. It may modify
its argument string, or copy part of it to static storage. However,
it will also delete trailing slashes, while the GNU version will
return the empty string if there is a trailing slash.

This change resolves the issue by including libgen.h, adopting POSIX
basename semantics. This should be a safe change for the following
reasons:

• The google-breakpad code, from which this code was derived, has
  also switched to the POSIX basename:
  https://chromium.googlesource.com/breakpad/breakpad/+/072f86ca83bb7138fe33f10b6380badd9ef7f065%5E%21/#F4

• The version of LulElf.cpp in mozglue/baseprofiler has also switched
  to the POSIX basename:
  https://hg.mozilla.org/mozilla-central/annotate/de1c3ae8df14cdb2c94a817b02dcffcb2cee12e2/mozglue/baseprofiler/lul/LulElf.cpp#l54

• The BaseFileName function is called only with paths to ELF files,
  never directories, so the paths will never contain a trailing
  slash, and the two versions of basename will behave identically.

Are you serioues, your gonna jump on a bug copy a patch and take credit for something you did not write? I can officially say I have seen it all now.

Comment 32

[mforney]

Are you serioues, your gonna jump on a bug copy a patch and take credit for something you did not write? I can officially say I have seen it all now.

I am not trying to take credit for anything, I don't care about that at all. I'm trying to get this issue fixed. It is pretty clear that to fix an error 'basename' not declared in this scope can be resolved by including the header that declares this function. By your logic, @gerald has also "stolen" Samuel's patch: https://hg.mozilla.org/mozilla-central/annotate/de1c3ae8df14cdb2c94a817b02dcffcb2cee12e2/mozglue/baseprofiler/lul/LulElf.cpp#l54

This issue has been open for 6 years, all the while patches have been available. It seems like the thing to do to move this forward is to submit the change for review through Mozilla's phabricator, as described in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch. Nobody has done this, so in the interest of getting this issue fixed, I have created D61047 after doing research about similar fixes for other files derived from the same code. If Samuel or anyone else who proposed a patch wants to propose their own patch for review, they are welcome to, and I will drop mine.

Comment 33

[mforney]

I believe I can amend my changeset to set the author to Samuel Holland, but I will have to check that he's okay with the commit message I have written.

Comment 34

[mforney]

(In reply to mforney from comment #33)

I believe I can amend my changeset to set the author to Samuel Holland, but I will have to check that he's okay with the commit message I have written.

This is now resolved.

Comment 35

[Julian Seward [:jseward]]

I will try to resolve this in the week 3-7 Feb 2020. Unfortunately I have no
bandwidth to deal with it this coming week.

Comment 36

[Julian Seward [:jseward]]

(In reply to mforney from comment #34)

I believe I can amend my changeset to set the author to Samuel Holland, but I will have to check that he's okay with the commit message I have written.

This is now resolved.

What is the resolution?

Comment 37

[mforney]

(In reply to Julian Seward [:jseward] from comment #36)

(In reply to mforney from comment #34)

I believe I can amend my changeset to set the author to Samuel Holland, but I will have to check that he's okay with the commit message I have written.

This is now resolved.

What is the resolution?

I contacted Samuel to see if he was okay with the commit message I wrote, and he said he was, so I amended the changeset in D61047 to list him as the author.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3096ca24124
Include libgen.h for basename r=jseward

Comment 39

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a3096ca24124