Closed Bug 1041962 Opened 7 years ago Closed 2 years ago

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

Categories

(Core :: Gecko Profiler, defect)

31 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: natanael.copa, Assigned: mforney)

Details

Attachments

(5 files)

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.
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.
Correction, it appears that _GNU_SOURCE is actually set so I don't know what is going on really.
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;
 }
(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.
Benoit, can you look into this? Seems like there's a patch and everything... :-)
Component: Untriaged → Gecko Profiler
Flags: needinfo?(bgirard)
Product: Firefox → Core
Isn't the point of basename that it deals with more complicated cases like escape sequences?
Flags: needinfo?(bgirard)
(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.
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
Sweet, thanks for checking.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bgirard)
Attached patch patchSplinter Review
Flags: needinfo?(bgirard)
Attachment #8516179 - Flags: review?(jseward)
Attachment #8516179 - Flags: review?(jseward) → review-
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.
Attachment #8516179 - Flags: review- → review+
I can reproduce the bug on current trunk. toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc has the same problem
(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/
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?
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.
Flags: needinfo?(bgirard)
Feel free to rebase it. I'm not fussy about the author field for such a small patch. Thanks for helping out!
Flags: needinfo?(bgirard)
Attached patch basename.patchSplinter Review
Rebase the previous patch for this bug. Please could someone push this to the Try server?
Attachment #8739744 - Flags: review?(jseward)
Sorry to bug you but please could you push this to the Try server for me?
Flags: needinfo?(bgirard)
Sorry for the delay:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a660c20fea
Flags: needinfo?(bgirard) → needinfo?(charles)
Charles, I am sorry for the excessive delay on this.  Do you
still wish to proceed with it?
Yes, please do.
Flags: needinfo?(charles)
(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?
(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?
Flags: needinfo?(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.
Flags: needinfo?(jseward)
(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.
This is the least intrusive patch and ensure it continues to work with all libc variants.
Attachment #8900998 - Flags: review?(jseward)
(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 "/".

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.

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).

Attachment #9122764 - Flags: review+

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:

Assignee: nobody → mforney
Status: NEW → ASSIGNED

(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:

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.

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.

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.

(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.

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.

(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?

(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.

Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3096ca24124
Include libgen.h for basename r=jseward
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Attachment #8739744 - Flags: review?(jseward)
Attachment #8900998 - Flags: review?(jseward)
You need to log in before you can comment on or make changes to this bug.