Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Romain Naour]

Hi Thomas,

Adding the gdb-patches ml and Rich Felker in Cc.

Le 10/09/2018 à 17:49, Thomas Petazzoni a écrit :
> Hello,
> 
> On Sun,  9 Sep 2018 18:37:50 +0200, Romain Naour wrote:
>> Use the same workaround [1] as gnulib use to get the original
>> definition of stat. Otherwise with musl toolchains, gnulib try to use
>> rpl_stat which is not defined.
>>
>> Fixes:
>> https://gitlab.com/free-electrons/toolchains-builder/-/jobs/95552308
>>
>> [1] http://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/stat.c?id=c9d72f69bd201a1ab31464d91f234ea1817fe0e1
>>
>> Signed-off-by: Romain Naour <romain.naour@gmail.com>
>> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> I am confused by this patch. Why do we need that? The <sys/stat.h> on
> my system doesn't test __need_system_sys_stat_h. Is this a workaround
> to force gnulib to not provide its own stat() replacement ?
> 
> Why is gnulib misbehaving here ? We have tons of gnulib related hacks
> in gdb.mk, and this start to pile up quite a bit. Why do we have all
> those gnulib issues with gdb ? Why not with tons of other packages that
> also use gnulib ?

There are too many questions here, I can't answer.
There are some (old) hack with coreutils like gl_cv_func_gettimeofday_clobber
which is in Buildroot since a long time. I can't tell for every gnulib based
packages...

> 
>> +Use the same workaround [1] as gnulib use to get the original
>> +definition of stat. Otherwise with musl toolchains, gnulib try to use
>> +rpl_stat which is not defined.
> 
> Well rpl_stat() is supposed to be implemented by gnulib. So basically
> gnulib tells gdb: please don't use stat() but my rpl_stat() wrapper,
> but then gnulib doesn't provide rpl_stat().
> 
> Any idea what's happening here ?

As far I can tell, the regression has been introduced by this commit:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=2441702a72f324e41a1624dc042b334f375e2d81

Best regards,
Romain

> 
> Thomas
> 

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Rich Felker]

On Mon, Sep 10, 2018 at 11:20:58PM +0200, Romain Naour wrote:
> Hi Thomas,
> 
> Adding the gdb-patches ml and Rich Felker in Cc.
> 
> Le 10/09/2018 à 17:49, Thomas Petazzoni a écrit :
> > Hello,
> > 
> > On Sun,  9 Sep 2018 18:37:50 +0200, Romain Naour wrote:
> >> Use the same workaround [1] as gnulib use to get the original
> >> definition of stat. Otherwise with musl toolchains, gnulib try to use
> >> rpl_stat which is not defined.
> >>
> >> Fixes:
> >> https://gitlab.com/free-electrons/toolchains-builder/-/jobs/95552308
> >>
> >> [1] http://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/stat.c?id=c9d72f69bd201a1ab31464d91f234ea1817fe0e1
> >>
> >> Signed-off-by: Romain Naour <romain.naour@gmail.com>
> >> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > 
> > I am confused by this patch. Why do we need that? The <sys/stat.h> on
> > my system doesn't test __need_system_sys_stat_h. Is this a workaround
> > to force gnulib to not provide its own stat() replacement ?
> > 
> > Why is gnulib misbehaving here ? We have tons of gnulib related hacks
> > in gdb.mk, and this start to pile up quite a bit. Why do we have all
> > those gnulib issues with gdb ? Why not with tons of other packages that
> > also use gnulib ?
> 
> There are too many questions here, I can't answer.
> There are some (old) hack with coreutils like gl_cv_func_gettimeofday_clobber
> which is in Buildroot since a long time. I can't tell for every gnulib based
> packages...
> 
> > 
> >> +Use the same workaround [1] as gnulib use to get the original
> >> +definition of stat. Otherwise with musl toolchains, gnulib try to use
> >> +rpl_stat which is not defined.
> > 
> > Well rpl_stat() is supposed to be implemented by gnulib. So basically
> > gnulib tells gdb: please don't use stat() but my rpl_stat() wrapper,
> > but then gnulib doesn't provide rpl_stat().
> > 
> > Any idea what's happening here ?
> 
> As far I can tell, the regression has been introduced by this commit:
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=2441702a72f324e41a1624dc042b334f375e2d81

I'm not aware of all the context, but it looks like different source
files disagree on whether gnulib has replaced stat or not -- the
gnulib source file thinks it hasn't, so the rpl_stat function isn't
defined, but gdb's common-utils-ipa.c file (or rather the gnulib
stat.h included into it?) thinks it has been replaced and is trying to
use the replacement. This is likely the result of an incorrect hack
somewhere. Do you know if it happens with upstream gdb and musl or
just in buildroot's package?

Rich

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Sergio Durigan Junior]

On Monday, September 10 2018, Rich Felker wrote:

> On Mon, Sep 10, 2018 at 11:20:58PM +0200, Romain Naour wrote:
>> Hi Thomas,
>> 
>> Adding the gdb-patches ml and Rich Felker in Cc.
>> 
>> Le 10/09/2018 à 17:49, Thomas Petazzoni a écrit :
>> > Hello,
>> > 
>> > On Sun,  9 Sep 2018 18:37:50 +0200, Romain Naour wrote:
>> >> Use the same workaround [1] as gnulib use to get the original
>> >> definition of stat. Otherwise with musl toolchains, gnulib try to use
>> >> rpl_stat which is not defined.
>> >>
>> >> Fixes:
>> >> https://gitlab.com/free-electrons/toolchains-builder/-/jobs/95552308
>> >>
>> >> [1] http://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/stat.c?id=c9d72f69bd201a1ab31464d91f234ea1817fe0e1
>> >>
>> >> Signed-off-by: Romain Naour <romain.naour@gmail.com>
>> >> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>> > 
>> > I am confused by this patch. Why do we need that? The <sys/stat.h> on
>> > my system doesn't test __need_system_sys_stat_h. Is this a workaround
>> > to force gnulib to not provide its own stat() replacement ?
>> > 
>> > Why is gnulib misbehaving here ? We have tons of gnulib related hacks
>> > in gdb.mk, and this start to pile up quite a bit. Why do we have all
>> > those gnulib issues with gdb ? Why not with tons of other packages that
>> > also use gnulib ?
>> 
>> There are too many questions here, I can't answer.
>> There are some (old) hack with coreutils like gl_cv_func_gettimeofday_clobber
>> which is in Buildroot since a long time. I can't tell for every gnulib based
>> packages...
>> 
>> > 
>> >> +Use the same workaround [1] as gnulib use to get the original
>> >> +definition of stat. Otherwise with musl toolchains, gnulib try to use
>> >> +rpl_stat which is not defined.
>> > 
>> > Well rpl_stat() is supposed to be implemented by gnulib. So basically
>> > gnulib tells gdb: please don't use stat() but my rpl_stat() wrapper,
>> > but then gnulib doesn't provide rpl_stat().
>> > 
>> > Any idea what's happening here ?
>> 
>> As far I can tell, the regression has been introduced by this commit:
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=2441702a72f324e41a1624dc042b334f375e2d81

This is happening because, before the commit mentioned above,
'common-utils.c' (which gets transformed into 'common-utils-ipa.c'
during the gdbserver build) wasn't calling 'stat'.  It doesn't seem like
a regression; it seems like a hidden problem that was uncovered by the
need of 'stat'.

I don't know why this problem is manifesting only when compiling IPA,
and not when compiling 'common-utils.c' during GDB's/gdbserver's build.

> I'm not aware of all the context, but it looks like different source
> files disagree on whether gnulib has replaced stat or not -- the
> gnulib source file thinks it hasn't, so the rpl_stat function isn't
> defined, but gdb's common-utils-ipa.c file (or rather the gnulib
> stat.h included into it?) thinks it has been replaced and is trying to
> use the replacement. This is likely the result of an incorrect hack
> somewhere. Do you know if it happens with upstream gdb and musl or
> just in buildroot's package?

Haven't really investigated, but be aware that there has been a recent
attempt at updating our local gnulib copy, which unfortunately
introduced a bunch of build failures and had to be reverted (even though
I don't really see how 'stat' could be affected).  I'm not sure if you
are using the latest git HEAD or not (it seems like you aren't, but I
decided to write this anyway); if that's the case, you might want to try
to rebuild with the latest HEAD (I reverted the gnulib update patch
today).

/me thinks a bit more...

I'm thinking here that this is similar to a problem we had recently,
which existed when cross-compiling GDB.  Basically, in this scenario,
gnulib's mechanism to detect cross-compilation was poor and lead to the
unneeded replacement of 'getcwd' in some systems that did have a working
'getcwd', and that was causing problems when running the cross GDB.  The
bug is here:

  https://sourceware.org/bugzilla/show_bug.cgi?id=23558

and the fix was to improve gnulib's machinery and make it a bit smarter
when detecting cross-compilation scenarios.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Thomas Petazzoni]

Hello,

On Mon, 10 Sep 2018 18:41:28 -0400, Rich Felker wrote:

> I'm not aware of all the context, but it looks like different source
> files disagree on whether gnulib has replaced stat or not -- the
> gnulib source file thinks it hasn't, so the rpl_stat function isn't
> defined, but gdb's common-utils-ipa.c file (or rather the gnulib
> stat.h included into it?) thinks it has been replaced and is trying to
> use the replacement. This is likely the result of an incorrect hack
> somewhere. Do you know if it happens with upstream gdb and musl or
> just in buildroot's package?

Well, Buildroot is using upstream musl and gdb. For both packages, we
have only very few patches:

  https://git.buildroot.org/buildroot/tree/package/musl/
  https://git.buildroot.org/buildroot/tree/package/gdb/8.1.1/

Note that we already have a number of gnulib related hacks in gdb.mk:

  https://git.buildroot.org/buildroot/tree/package/gdb/gdb.mk#n77

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Pedro Alves]

On 09/11/2018 01:38 AM, Sergio Durigan Junior wrote:

> This is happening because, before the commit mentioned above,
> 'common-utils.c' (which gets transformed into 'common-utils-ipa.c'
> during the gdbserver build) wasn't calling 'stat'.  It doesn't seem like
> a regression; it seems like a hidden problem that was uncovered by the
> need of 'stat'.
> 
> I don't know why this problem is manifesting only when compiling IPA,
> and not when compiling 'common-utils.c' during GDB's/gdbserver's build.

Because the IPA doesn't link with gnulib.  And the answer to that wouldn't
be as simple as "just link it in", because the IPA objects are supposed
to be compiled with -fPIC and -fvisibility=hidden.  So we'd need a third
build of gnulib for the IPA.

It doesn't seem like this code that calls stat (is_regular_file?) is
useful for the IPA, so a quicker/simpler fix would be to simply move
that function out of common-utils.c into some other file that is not
shared with the IPA.

Thanks,
Pedro Alves

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Pedro Alves]

On 09/11/2018 01:38 AM, Sergio Durigan Junior wrote:
> I'm thinking here that this is similar to a problem we had recently,
> which existed when cross-compiling GDB.  Basically, in this scenario,
> gnulib's mechanism to detect cross-compilation was poor and lead to the
> unneeded replacement of 'getcwd' in some systems that did have a working
> 'getcwd', and that was causing problems when running the cross GDB.
>  The
> bug is here:
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=23558
> 
> and the fix was to improve gnulib's machinery and make it a bit smarter
> when detecting cross-compilation scenarios.

Just to add a bit more color here: for some cases, gnulib needs to
be able to run things on the target in order to detect whether the
function being checked needs replacement:

 ./m4/getcwd-abort-bug.m4:     AC_RUN_IFELSE(
 ./m4/getcwd.m4:     [AC_RUN_IFELSE([AC_LANG_PROGRAM([[
 ./m4/getcwd-path-max.m4:     AC_RUN_IFELSE(

If you're cross compiling, then obviously you can't run things on the
target.  So all you can do, is take a guess, and if you don't know
anything about the system, the best is to take a conservative guess of
"needs fixing/replacing".  The gnulib fix pointed out is simply adding a
few targets to a whitelist of systems that don't need the fix/replacement,
so that when cross compiling for those systems, gnulib doesn't take the
default conservative approach.

Seems like the stat checks in gnulib also rely on running code,
and thus this could indeed be a similar case:

 ./m4/fstatat.m4:      [AC_RUN_IFELSE(
 ./m4/lstat.m4:     AC_RUN_IFELSE(
 ./m4/stat.m4:         AC_RUN_IFELSE(

Though from a quick peek, if the target triplet include "linux"
or "gnu", it should guess OK:

           [case "$host_os" in
                               # Guess yes on Linux systems.
              linux-* | linux) gl_cv_func_stat_file_slash="guessing yes" ;;
                               # Guess yes on glibc systems.
              *-gnu* | gnu*)   gl_cv_func_stat_file_slash="guessing yes" ;;
                               # If we don't know, assume the worst.
              *)               gl_cv_func_stat_file_slash="guessing no" ;;
            esac

So there might well be some other reason gnulib determines that stat
must be replaced on musl.  The ideal would be if no replacement were
necessary at all.

Thanks,
Pedro Alves

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Rich Felker]

On Tue, Sep 11, 2018 at 10:51:20AM +0100, Pedro Alves wrote:
> On 09/11/2018 01:38 AM, Sergio Durigan Junior wrote:
> 
> > This is happening because, before the commit mentioned above,
> > 'common-utils.c' (which gets transformed into 'common-utils-ipa.c'
> > during the gdbserver build) wasn't calling 'stat'.  It doesn't seem like
> > a regression; it seems like a hidden problem that was uncovered by the
> > need of 'stat'.
> > 
> > I don't know why this problem is manifesting only when compiling IPA,
> > and not when compiling 'common-utils.c' during GDB's/gdbserver's build.
> 
> Because the IPA doesn't link with gnulib.  And the answer to that wouldn't
> be as simple as "just link it in", because the IPA objects are supposed
> to be compiled with -fPIC and -fvisibility=hidden.  So we'd need a third
> build of gnulib for the IPA.
> 
> It doesn't seem like this code that calls stat (is_regular_file?) is
> useful for the IPA, so a quicker/simpler fix would be to simply move
> that function out of common-utils.c into some other file that is not
> shared with the IPA.

Eew, I just looked up what IPA is. Yes, it's buggy to be including
gnulib headers in files used for this unless you have another copy
compiled with hidden visibility. Alternatively, you could put all the
gnulib files into a static archive and link with
-Wl,--exclude-libs=ALL to make them hidden at link-time, without
having to compile them again.

As an aside, the whole idea of running in-process code
referencing/using functions in the process being debugged (like stat
in libc) seems fragile and broken; perhaps aside from this
inadvertently-added use in is_regular_file, there is no such code in
the IPA stuff?

Rich

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Pedro Alves]

On 09/11/2018 03:13 PM, Rich Felker wrote:

> As an aside, the whole idea of running in-process code
> referencing/using functions in the process being debugged (like stat
> in libc) seems fragile and broken; perhaps aside from this
> inadvertently-added use in is_regular_file, there is no such code in
> the IPA stuff?

There is (or there should be) no such code in the fast path, which is
actually collecting tracepoints -- there are no syscalls in that path.
If there were, the whole point of the IPA would be thrown out the window.
There's some in setup code, but it hasn't been problematic.

Thanks,
Pedro Alves

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Sergio Durigan Junior]

On Tuesday, September 11 2018, Pedro Alves wrote:

> On 09/11/2018 01:38 AM, Sergio Durigan Junior wrote:
>
>> This is happening because, before the commit mentioned above,
>> 'common-utils.c' (which gets transformed into 'common-utils-ipa.c'
>> during the gdbserver build) wasn't calling 'stat'.  It doesn't seem like
>> a regression; it seems like a hidden problem that was uncovered by the
>> need of 'stat'.
>> 
>> I don't know why this problem is manifesting only when compiling IPA,
>> and not when compiling 'common-utils.c' during GDB's/gdbserver's build.
>
> Because the IPA doesn't link with gnulib.  And the answer to that wouldn't
> be as simple as "just link it in", because the IPA objects are supposed
> to be compiled with -fPIC and -fvisibility=hidden.  So we'd need a third
> build of gnulib for the IPA.

That explains it.  It seems strange to me that we still include the
gnulib headers when compiling IPA; I confess I just assumed IPA was
linking with gnulib because of this.

> It doesn't seem like this code that calls stat (is_regular_file?) is
> useful for the IPA, so a quicker/simpler fix would be to simply move
> that function out of common-utils.c into some other file that is not
> shared with the IPA.

It's not useful for IPA; it could be moved to common/filestuff.c, for
example.  But IMHO, we should probably have an explicit file just for
IPA, because otherwise we'll forget about this restriction and re-add
some 'stat' calls to common-utils.c.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Pedro Alves]

On 09/11/2018 07:21 PM, Sergio Durigan Junior wrote:
> On Tuesday, September 11 2018, Pedro Alves wrote:
> 
>> On 09/11/2018 01:38 AM, Sergio Durigan Junior wrote:
>>
>>> This is happening because, before the commit mentioned above,
>>> 'common-utils.c' (which gets transformed into 'common-utils-ipa.c'
>>> during the gdbserver build) wasn't calling 'stat'.  It doesn't seem like
>>> a regression; it seems like a hidden problem that was uncovered by the
>>> need of 'stat'.
>>>
>>> I don't know why this problem is manifesting only when compiling IPA,
>>> and not when compiling 'common-utils.c' during GDB's/gdbserver's build.
>>
>> Because the IPA doesn't link with gnulib.  And the answer to that wouldn't
>> be as simple as "just link it in", because the IPA objects are supposed
>> to be compiled with -fPIC and -fvisibility=hidden.  So we'd need a third
>> build of gnulib for the IPA.
> 
> That explains it.  It seems strange to me that we still include the
> gnulib headers when compiling IPA; I confess I just assumed IPA was
> linking with gnulib because of this.

In order _not_ to include the gnulib headers, that would mean that we'd
be OK with going back to making sure we do any necessary portability
autoconf/#ifdefery ourselves in all of common and gdbserver code that is
used by the IPA, which doesn't sound very appealing to me.  
I'd think it better / simpler maintenance-wise going forward, to work in the
direction of linking with gnulib if/when we find a need.  

The IPA has so few dependencies by design that in practice just using the
headers should be fine, with gnulib ironing-out any header-only portability
issues.  It's quite likely that a port that can use the IPA won't really need
any gnu_foo replacement function.  And if it does, then it should be
better to use gnulib's replacements instead of duplicating what gnulib
already does, I'd think.

> 
>> It doesn't seem like this code that calls stat (is_regular_file?) is
>> useful for the IPA, so a quicker/simpler fix would be to simply move
>> that function out of common-utils.c into some other file that is not
>> shared with the IPA.
> 
> It's not useful for IPA; it could be moved to common/filestuff.c, for
> example.  

Right.

> But IMHO, we should probably have an explicit file just for
> IPA, because otherwise we'll forget about this restriction and re-add
> some 'stat' calls to common-utils.c.

I'm afraid I don't understand what you mean here.

Thanks,
Pedro Alves

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Sergio Durigan Junior]

On Wednesday, September 12 2018, Pedro Alves wrote:

> On 09/11/2018 07:21 PM, Sergio Durigan Junior wrote:
>> On Tuesday, September 11 2018, Pedro Alves wrote:
>> 
>>> On 09/11/2018 01:38 AM, Sergio Durigan Junior wrote:
>>>
>>>> This is happening because, before the commit mentioned above,
>>>> 'common-utils.c' (which gets transformed into 'common-utils-ipa.c'
>>>> during the gdbserver build) wasn't calling 'stat'.  It doesn't seem like
>>>> a regression; it seems like a hidden problem that was uncovered by the
>>>> need of 'stat'.
>>>>
>>>> I don't know why this problem is manifesting only when compiling IPA,
>>>> and not when compiling 'common-utils.c' during GDB's/gdbserver's build.
>>>
>>> Because the IPA doesn't link with gnulib.  And the answer to that wouldn't
>>> be as simple as "just link it in", because the IPA objects are supposed
>>> to be compiled with -fPIC and -fvisibility=hidden.  So we'd need a third
>>> build of gnulib for the IPA.
>> 
>> That explains it.  It seems strange to me that we still include the
>> gnulib headers when compiling IPA; I confess I just assumed IPA was
>> linking with gnulib because of this.
>
> In order _not_ to include the gnulib headers, that would mean that we'd
> be OK with going back to making sure we do any necessary portability
> autoconf/#ifdefery ourselves in all of common and gdbserver code that is
> used by the IPA, which doesn't sound very appealing to me.  
> I'd think it better / simpler maintenance-wise going forward, to work in the
> direction of linking with gnulib if/when we find a need.  
>
> The IPA has so few dependencies by design that in practice just using the
> headers should be fine, with gnulib ironing-out any header-only portability
> issues.  It's quite likely that a port that can use the IPA won't really need
> any gnu_foo replacement function.  And if it does, then it should be
> better to use gnulib's replacements instead of duplicating what gnulib
> already does, I'd think.

I guess I don't really understand what's going on behind the curtains
here, then.  I thought that, since IPA doesn't link with gnulib, then we
shouldn't be compiling it using "... -I./../gnulib/import
-Ibuild-gnulib-gdbserver/import ...", since it can be confusing for the
reader to determine "oh, gnulib's header files are being included, but
the library is not actually *linking* with gnulib".  I understand that
it'd be simpler to just build a third gnulib just for linking with IPA,
but it seems like we're not going to do that, at least not right now.

Anyway, this is all very unimportant and I don't want to waste people's
time; I'm just trying to understand and to express the difficulty that I
had while examining the logs (i.e., determining that IPA wasn't linking
with gnulib, despite the inclusion of the headers).

>> 
>>> It doesn't seem like this code that calls stat (is_regular_file?) is
>>> useful for the IPA, so a quicker/simpler fix would be to simply move
>>> that function out of common-utils.c into some other file that is not
>>> shared with the IPA.
>> 
>> It's not useful for IPA; it could be moved to common/filestuff.c, for
>> example.  
>
> Right.
>
>> But IMHO, we should probably have an explicit file just for
>> IPA, because otherwise we'll forget about this restriction and re-add
>> some 'stat' calls to common-utils.c.
>
> I'm afraid I don't understand what you mean here.

This problem happened because (a) IPA doesn't link with gnulib, (b) IPA
uses common-utils.c, and (c) common-utils.c calls 'stat', which, in a
cross-compilation environment, gets replaced by gnulib's 'rpl_stat'.  We
want to solve the problem by tackling (c) and moving the 'stat' call to
another file that is not used by IPA.

My point is that, some time in the future, someone might not remember/be
aware of this problem and reintroduce a call to 'stat' on
common-utils.c, which would reintroduce this problem (assuming nothing
is done on gnulib's side to fix this).

I wrote the "proposal" above without completely understanding the
problem.  Indeed, it doesn't make sense to have a separate, IPA-specific
file for common-utils.c.  I guess the best approach here would be,
again, to compile a separate version of gnulib for IPA.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Sergio Durigan Junior]

There is no reason for 'is_regular_file' to be in common-utils.c; it
belongs to 'filestuff.c'.  This commit moves the function definition
and its prototype to the appropriate files.

The motivation behind this move is a failure that happens on certain
cross-compilation environments when compiling the IPA library, due to
the way gnulib probes the need for a 'stat' call replacement.  Because
configure checks when cross-compiling are more limited, gnulib decides
that it needs to substitute the 'stat' calls its own 'rpl_stat';
however, the IPA library doesn't link with gnulib, which leads to an
error when compiling 'common-utils.c':

  ...
  /opt/x86-core2--musl--bleeding-edge-2018.09-1/bin/i686-buildroot-linux-musl-g++  -shared -fPIC -Wl,--soname=libinproctrace.so -Wl,--no-undefined -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Os      -I. -I. -I./../common -I./../regformats -I./.. -I./../../include -I./../gnulib/import -Ibuild-gnulib-gdbserver/import -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized  -DGDBSERVER \
   -Wl,--dynamic-list=./proc-service.list -o libinproctrace.so ax-ipa.o common-utils-ipa.o errors-ipa.o format-ipa.o print-utils-ipa.o regcache-ipa.o remote-utils-ipa.o rsp-low-ipa.o tdesc-ipa.o tracepoint-ipa.o utils-ipa.o vec-ipa.o linux-i386-ipa.o linux-x86-tdesc-ipa.o arch/i386-ipa.o -ldl -pthread
  /opt/x86-core2--musl--bleeding-edge-2018.09-1/lib/gcc/i686-buildroot-linux-musl/8.2.0/../../../../i686-buildroot-linux-musl/bin/ld: common-utils-ipa.o: in function `is_regular_file(char const*, int*)':
  common-utils.c:(.text+0x695): undefined reference to `rpl_stat'
  collect2: error: ld returned 1 exit status
  Makefile:413: recipe for target 'libinproctrace.so' failed
  make[1]: *** [libinproctrace.so] Error 1
  ...

More details can also be found at:

  https://sourceware.org/ml/gdb-patches/2018-09/msg00304.html

The most simple fix for this problem is to move 'is_regular_file' to
'filestuff.c', which is not used by IPA.  This ends up making the
files more logically organized as well, since 'is_regular_file' is a
file operation.

No regressions found.

gdb/ChangeLog:
2018-09-12  Sergio Durigan Junior  <sergiodj@redhat.com>

	* common/common-utils.c: Don't include '<sys/stat.h>'.
	(is_regular_file): Move to...
	* common/filestuff.c (is_regular_file): ... here.
	* common/common-utils.h (is_regular_file): Move to...
	* common/filestuff.h (is_regular_file): ... here.
---
 gdb/common/common-utils.c | 32 --------------------------------
 gdb/common/common-utils.h |  5 -----
 gdb/common/filestuff.c    | 31 +++++++++++++++++++++++++++++++
 gdb/common/filestuff.h    |  5 +++++
 4 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 8d839d10fa..24b3936f3d 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -20,7 +20,6 @@
 #include "common-defs.h"
 #include "common-utils.h"
 #include "host-defs.h"
-#include <sys/stat.h>
 #include <ctype.h>
 
 /* The xmalloc() (libiberty.h) family of memory management routines.
@@ -412,37 +411,6 @@ stringify_argv (const std::vector<char *> &args)
 
 /* See common/common-utils.h.  */
 
-bool
-is_regular_file (const char *name, int *errno_ptr)
-{
-  struct stat st;
-  const int status = stat (name, &st);
-
-  /* Stat should never fail except when the file does not exist.
-     If stat fails, analyze the source of error and return true
-     unless the file does not exist, to avoid returning false results
-     on obscure systems where stat does not work as expected.  */
-
-  if (status != 0)
-    {
-      if (errno != ENOENT)
-	return true;
-      *errno_ptr = ENOENT;
-      return false;
-    }
-
-  if (S_ISREG (st.st_mode))
-    return true;
-
-  if (S_ISDIR (st.st_mode))
-    *errno_ptr = EISDIR;
-  else
-    *errno_ptr = EINVAL;
-  return false;
-}
-
-/* See common/common-utils.h.  */
-
 ULONGEST
 align_up (ULONGEST v, int n)
 {
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 7bc6e90f05..a961514fd6 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -146,11 +146,6 @@ in_inclusive_range (T value, T low, T high)
   return value >= low && value <= high;
 }
 
-/* Return true if the file NAME exists and is a regular file.
-   If the result is false then *ERRNO_PTR is set to a useful value assuming
-   we're expecting a regular file.  */
-extern bool is_regular_file (const char *name, int *errno_ptr);
-
 /* Ensure that V is aligned to an N byte boundary (B's assumed to be a
    power of 2).  Round up/down when necessary.  Examples of correct
    use include:
diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index f5a754ffa6..fa10165a7c 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -417,3 +417,34 @@ make_cleanup_close (int fd)
   *saved_fd = fd;
   return make_cleanup_dtor (do_close_cleanup, saved_fd, xfree);
 }
+
+/* See common/filestuff.h.  */
+
+bool
+is_regular_file (const char *name, int *errno_ptr)
+{
+  struct stat st;
+  const int status = stat (name, &st);
+
+  /* Stat should never fail except when the file does not exist.
+     If stat fails, analyze the source of error and return true
+     unless the file does not exist, to avoid returning false results
+     on obscure systems where stat does not work as expected.  */
+
+  if (status != 0)
+    {
+      if (errno != ENOENT)
+	return true;
+      *errno_ptr = ENOENT;
+      return false;
+    }
+
+  if (S_ISREG (st.st_mode))
+    return true;
+
+  if (S_ISDIR (st.st_mode))
+    *errno_ptr = EISDIR;
+  else
+    *errno_ptr = EINVAL;
+  return false;
+}
diff --git a/gdb/common/filestuff.h b/gdb/common/filestuff.h
index 1a09c729f6..e9328f5358 100644
--- a/gdb/common/filestuff.h
+++ b/gdb/common/filestuff.h
@@ -117,4 +117,9 @@ struct gdb_dir_deleter
 
 typedef std::unique_ptr<DIR, gdb_dir_deleter> gdb_dir_up;
 
+/* Return true if the file NAME exists and is a regular file.
+   If the result is false then *ERRNO_PTR is set to a useful value assuming
+   we're expecting a regular file.  */
+extern bool is_regular_file (const char *name, int *errno_ptr);
+
 #endif /* FILESTUFF_H */
-- 
2.17.1

Re: [Buildroot] [PATCH 2/2] package/gdb: use stat() privided by the system

[Pedro Alves]

On 09/12/2018 06:11 PM, Sergio Durigan Junior wrote:
> On Wednesday, September 12 2018, Pedro Alves wrote:
> 
>> On 09/11/2018 07:21 PM, Sergio Durigan Junior wrote:
>>> On Tuesday, September 11 2018, Pedro Alves wrote:
>>>
>>>> On 09/11/2018 01:38 AM, Sergio Durigan Junior wrote:
>>>>
>>>>> This is happening because, before the commit mentioned above,
>>>>> 'common-utils.c' (which gets transformed into 'common-utils-ipa.c'
>>>>> during the gdbserver build) wasn't calling 'stat'.  It doesn't seem like
>>>>> a regression; it seems like a hidden problem that was uncovered by the
>>>>> need of 'stat'.
>>>>>
>>>>> I don't know why this problem is manifesting only when compiling IPA,
>>>>> and not when compiling 'common-utils.c' during GDB's/gdbserver's build.
>>>>
>>>> Because the IPA doesn't link with gnulib.  And the answer to that wouldn't
>>>> be as simple as "just link it in", because the IPA objects are supposed
>>>> to be compiled with -fPIC and -fvisibility=hidden.  So we'd need a third
>>>> build of gnulib for the IPA.
>>>
>>> That explains it.  It seems strange to me that we still include the
>>> gnulib headers when compiling IPA; I confess I just assumed IPA was
>>> linking with gnulib because of this.
>>
>> In order _not_ to include the gnulib headers, that would mean that we'd
>> be OK with going back to making sure we do any necessary portability
>> autoconf/#ifdefery ourselves in all of common and gdbserver code that is
>> used by the IPA, which doesn't sound very appealing to me.  
>> I'd think it better / simpler maintenance-wise going forward, to work in the
>> direction of linking with gnulib if/when we find a need.  
>>
>> The IPA has so few dependencies by design that in practice just using the
>> headers should be fine, with gnulib ironing-out any header-only portability
>> issues.  It's quite likely that a port that can use the IPA won't really need
>> any gnu_foo replacement function.  And if it does, then it should be
>> better to use gnulib's replacements instead of duplicating what gnulib
>> already does, I'd think.
> 
> I guess I don't really understand what's going on behind the curtains
> here, then.  I thought that, since IPA doesn't link with gnulib, then we
> shouldn't be compiling it using "... -I./../gnulib/import
> -Ibuild-gnulib-gdbserver/import ...", since it can be confusing for the
> reader to determine "oh, gnulib's header files are being included, but
> the library is not actually *linking* with gnulib".  I understand that
> it'd be simpler to just build a third gnulib just for linking with IPA,
> but it seems like we're not going to do that, at least not right now.

gnulib provides:

 #1 - replacement headers for broken system headers.
 #2 - replacement implementations of system/libc functions, for broken
      systems.
 #3 - extra utility functions 

For #2, the replacement functions go by the same name as the system
function, but prefixed by "rpl_".  "#define foo rpl_foo" is used to 
provide the illusion that you're calling the original function.
If all you need are replacement headers, #1 above, e.g., because you
need a guarantee to have a C99-or-later-conforming stdint.h, and never call
any of the functions that need to be replaced (#2), or any of the extra
function (#3 above), then there's nothing of interest to link with in
the generated libgnu.a.  If we move the stat call elsewhere not used by the
IPA, then linking the IPA with gnulib would be effectively a no-op.
Until/unless someone adds some call that is resolved to some rpl_foo function
that is really necessary on some port, that is.  When/if that happens, we
must necessarily consider whether to build a gnulib for the IPA, of course.
Until then, it doesn't seem worth it to me to go through the effort in
order to link in a no-op library.

But if someone wants to give it a try, then please, by all means,
I'd definitely be happy!  It isn't impossible at all.  It just seemed
like the "get stat out of the way" solution would be quick enough
to handle, and harmless otherwise.

> 
> Anyway, this is all very unimportant and I don't want to waste people's
> time; I'm just trying to understand and to express the difficulty that I
> had while examining the logs (i.e., determining that IPA wasn't linking
> with gnulib, despite the inclusion of the headers).

It's just one of those things that you know after experiencing it
a few times.  If the linker complains about some library symbol, the
first thing to look at is whether you're actually including the library
in the link.

Thanks,
Pedro Alves

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Pedro Alves]

On 09/12/2018 06:31 PM, Sergio Durigan Junior wrote:

> The most simple fix for this problem is to move 'is_regular_file' to
> 'filestuff.c', which is not used by IPA.  This ends up making the
> files more logically organized as well, since 'is_regular_file' is a
> file operation.
> 
> No regressions found.
> 
> gdb/ChangeLog:
> 2018-09-12  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* common/common-utils.c: Don't include '<sys/stat.h>'.
> 	(is_regular_file): Move to...
> 	* common/filestuff.c (is_regular_file): ... here.
> 	* common/common-utils.h (is_regular_file): Move to...
> 	* common/filestuff.h (is_regular_file): ... here.

OK.

Thanks,
Pedro Alves

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Sergio Durigan Junior]

On Wednesday, September 12 2018, Pedro Alves wrote:

> On 09/12/2018 06:31 PM, Sergio Durigan Junior wrote:
>
>> The most simple fix for this problem is to move 'is_regular_file' to
>> 'filestuff.c', which is not used by IPA.  This ends up making the
>> files more logically organized as well, since 'is_regular_file' is a
>> file operation.
>> 
>> No regressions found.
>> 
>> gdb/ChangeLog:
>> 2018-09-12  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* common/common-utils.c: Don't include '<sys/stat.h>'.
>> 	(is_regular_file): Move to...
>> 	* common/filestuff.c (is_regular_file): ... here.
>> 	* common/common-utils.h (is_regular_file): Move to...
>> 	* common/filestuff.h (is_regular_file): ... here.
>
> OK.

Thanks, pushed.

3c025cfe5efc44eb4dfb03b53dca28e75096dd1e

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Romain Naour]

Hi All,

Le 12/09/2018 à 19:59, Sergio Durigan Junior a écrit :
> On Wednesday, September 12 2018, Pedro Alves wrote:
> 
>> On 09/12/2018 06:31 PM, Sergio Durigan Junior wrote:
>>
>>> The most simple fix for this problem is to move 'is_regular_file' to
>>> 'filestuff.c', which is not used by IPA.  This ends up making the
>>> files more logically organized as well, since 'is_regular_file' is a
>>> file operation.
>>>
>>> No regressions found.
>>>
>>> gdb/ChangeLog:
>>> 2018-09-12  Sergio Durigan Junior  <sergiodj@redhat.com>
>>>
>>> 	* common/common-utils.c: Don't include '<sys/stat.h>'.
>>> 	(is_regular_file): Move to...
>>> 	* common/filestuff.c (is_regular_file): ... here.
>>> 	* common/common-utils.h (is_regular_file): Move to...
>>> 	* common/filestuff.h (is_regular_file): ... here.
>>
>> OK.
> 
> Thanks, pushed.

Thanks for the patch!
It should be backported to 8.1.1 and 8.2 release.

Best regards,
Romain

> 
> 3c025cfe5efc44eb4dfb03b53dca28e75096dd1e
> 

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Tom Tromey]

>>>>> "Romain" == Romain Naour <romain.naour@smile.fr> writes:

Romain> It should be backported to 8.1.1 and 8.2 release.

FWIW I doubt there will be another 8.1.x release.

Tom

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Sergio Durigan Junior]

On Friday, September 14 2018, Tom Tromey wrote:

>>>>>> "Romain" == Romain Naour <romain.naour@smile.fr> writes:
>
> Romain> It should be backported to 8.1.1 and 8.2 release.
>
> FWIW I doubt there will be another 8.1.x release.

Me too.  I can backport it to 8.2 if wanted.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Romain Naour]

Le 14/09/2018 à 23:48, Sergio Durigan Junior a écrit :
> On Friday, September 14 2018, Tom Tromey wrote:
> 
>>>>>>> "Romain" == Romain Naour <romain.naour@smile.fr> writes:
>>
>> Romain> It should be backported to 8.1.1 and 8.2 release.
>>
>> FWIW I doubt there will be another 8.1.x release.
> 
> Me too.  I can backport it to 8.2 if wanted.

Yes please for the upcoming 8.2.1.

Best regards,
Romain

> 
> Thanks,
> 

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Joel Brobecker]

Hello Romain,

> > Me too.  I can backport it to 8.2 if wanted.
> 
> Yes please for the upcoming 8.2.1.

Once a release has been made off a give branch, there are a couple
of additional conditions before we can backport that change to
that branch:

  - The patch needs to be safe, and you need approval from a Global
    Maintainer to do so;

    I looked at the patch, and although I was nervous about
    the removal of the <stat.h> #include at first, I checked
    the contents of the file, and I am reasonably certain that
    there isn't any other code that might needed on some obscure
    system. So you have my OK for the patch to be backported to
    the gdb-8.2-branch.

  - And the commit should have a corresponding GDB PR number
    (https://sourceware.org/bugzilla/); the PR number acts as
    a reference for "what's changed" in each corrective release,
    so the clearer the explanation in the description, the better.

    If you are the one motivated for the backport, it would be
    helpful if you could take care of creating the PR. Sergio
    offered to then backport it for you, but I can also help
    with that.

Thank you,
-- 
Joel

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Romain Naour]

Hi Joel, All,

Le 15/09/2018 à 00:09, Joel Brobecker a écrit :
> Hello Romain,
> 
>>> Me too.  I can backport it to 8.2 if wanted.
>>
>> Yes please for the upcoming 8.2.1.
> 
> Once a release has been made off a give branch, there are a couple
> of additional conditions before we can backport that change to
> that branch:
> 
>   - The patch needs to be safe, and you need approval from a Global
>     Maintainer to do so;
> 
>     I looked at the patch, and although I was nervous about
>     the removal of the <stat.h> #include at first, I checked
>     the contents of the file, and I am reasonably certain that
>     there isn't any other code that might needed on some obscure
>     system. So you have my OK for the patch to be backported to
>     the gdb-8.2-branch.

Ok, thanks.
Note that Sergio is the original author of this part of the code introduced with
gdb 8.2 and backported to gdb 8.1.1 [1].

> 
>   - And the commit should have a corresponding GDB PR number
>     (https://sourceware.org/bugzilla/); the PR number acts as
>     a reference for "what's changed" in each corrective release,
>     so the clearer the explanation in the description, the better.
> 
>     If you are the one motivated for the backport, it would be
>     helpful if you could take care of creating the PR. Sergio
>     offered to then backport it for you, but I can also help
>     with that.

I'm agree on the principle but the change from gdb 8.2 introducing the issue has
been backported to gdb 8.1.x [1] without such GDB PR number (as far I can see).
IIUC, all patches backported to a stable branch must have a GDB PR number.
This is not always the case.

Without the fix provided by Sergio (Thanks!) we can't build gdb with a musl
toolchain, so gdb 8.1.x and gdb 8.2.x remain unfixed.

I'll create the GDB PR but all explanations of the issue have been provided on
the mailing list by gdb developers and maintainers.

[1]
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=2441702a72f324e41a1624dc042b334f375e2d81

Best regards,
Romain

> 
> Thank you,
> 

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Romain Naour]

Le 15/09/2018 à 15:15, Romain Naour a écrit :
> Hi Joel, All,

> I'll create the GDB PR but all explanations of the issue have been provided on
> the mailing list by gdb developers and maintainers.
> 

https://sourceware.org/bugzilla/show_bug.cgi?id=23663

Best regards,
Romain

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Sergio Durigan Junior]

On Saturday, September 15 2018, Romain Naour wrote:

>>   - And the commit should have a corresponding GDB PR number
>>     (https://sourceware.org/bugzilla/); the PR number acts as
>>     a reference for "what's changed" in each corrective release,
>>     so the clearer the explanation in the description, the better.
>> 
>>     If you are the one motivated for the backport, it would be
>>     helpful if you could take care of creating the PR. Sergio
>>     offered to then backport it for you, but I can also help
>>     with that.
>
> I'm agree on the principle but the change from gdb 8.2 introducing the issue has
> been backported to gdb 8.1.x [1] without such GDB PR number (as far I can see).
> IIUC, all patches backported to a stable branch must have a GDB PR number.
> This is not always the case.

It is always the case.  Joel kindly reminded me (back then) that I had
forgotten to create a PR for this backport:

  https://sourceware.org/ml/gdb-patches/2018-03/msg00001.html

And I did it subsequently:

  https://sourceware.org/bugzilla/show_bug.cgi?id=22907

Unfortunately, it was not possible to reference the PR number in the
commit log anymore, but it was included in the official release note:

  https://sourceware.org/ml/gdb-announce/2018/msg00002.html

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Sergio Durigan Junior]

On Saturday, September 15 2018, Romain Naour wrote:

> Le 15/09/2018 à 15:15, Romain Naour a écrit :
>> Hi Joel, All,
>
>> I'll create the GDB PR but all explanations of the issue have been provided on
>> the mailing list by gdb developers and maintainers.
>> 
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=23663

Thanks, I've now pushed the backported patch to the GDB 8.2 branch.
I've also set the Target Milestone of this PR to "8.2.1"; Joel, please
feel free to correct this if it is wrong.

5de69bdbd0bbd7941b4cd93d4571f5e22cdb28be

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Romain Naour]

Hi Sergio,

Le 15/09/2018 à 22:35, Sergio Durigan Junior a écrit :
> On Saturday, September 15 2018, Romain Naour wrote:
> 
>>>   - And the commit should have a corresponding GDB PR number
>>>     (https://sourceware.org/bugzilla/); the PR number acts as
>>>     a reference for "what's changed" in each corrective release,
>>>     so the clearer the explanation in the description, the better.
>>>
>>>     If you are the one motivated for the backport, it would be
>>>     helpful if you could take care of creating the PR. Sergio
>>>     offered to then backport it for you, but I can also help
>>>     with that.
>>
>> I'm agree on the principle but the change from gdb 8.2 introducing the issue has
>> been backported to gdb 8.1.x [1] without such GDB PR number (as far I can see).
>> IIUC, all patches backported to a stable branch must have a GDB PR number.
>> This is not always the case.
> 
> It is always the case.  Joel kindly reminded me (back then) that I had
> forgotten to create a PR for this backport:
> 
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00001.html
> 
> And I did it subsequently:
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=22907
> 
> Unfortunately, it was not possible to reference the PR number in the
> commit log anymore, but it was included in the official release note:
> 
>   https://sourceware.org/ml/gdb-announce/2018/msg00002.html

Sorry, I missed the reference to the PR.
Thanks for the links.

Best regards,
Romain

> 
> Thanks,
> 

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Sergio Durigan Junior]

On Saturday, September 15 2018, Romain Naour wrote:

> Hi Sergio,
>
> Le 15/09/2018 à 22:35, Sergio Durigan Junior a écrit :
>> On Saturday, September 15 2018, Romain Naour wrote:
>> 
>>>>   - And the commit should have a corresponding GDB PR number
>>>>     (https://sourceware.org/bugzilla/); the PR number acts as
>>>>     a reference for "what's changed" in each corrective release,
>>>>     so the clearer the explanation in the description, the better.
>>>>
>>>>     If you are the one motivated for the backport, it would be
>>>>     helpful if you could take care of creating the PR. Sergio
>>>>     offered to then backport it for you, but I can also help
>>>>     with that.
>>>
>>> I'm agree on the principle but the change from gdb 8.2 introducing the issue has
>>> been backported to gdb 8.1.x [1] without such GDB PR number (as far I can see).
>>> IIUC, all patches backported to a stable branch must have a GDB PR number.
>>> This is not always the case.
>> 
>> It is always the case.  Joel kindly reminded me (back then) that I had
>> forgotten to create a PR for this backport:
>> 
>>   https://sourceware.org/ml/gdb-patches/2018-03/msg00001.html
>> 
>> And I did it subsequently:
>> 
>>   https://sourceware.org/bugzilla/show_bug.cgi?id=22907
>> 
>> Unfortunately, it was not possible to reference the PR number in the
>> commit log anymore, but it was included in the official release note:
>> 
>>   https://sourceware.org/ml/gdb-announce/2018/msg00002.html
>
> Sorry, I missed the reference to the PR.
> Thanks for the links.

No problem at all, it certainly wasn't obvious.  Thanks for reporting
the bug and filing the PR :-).

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Move 'is_regular_file' from common-utils.c to filestuff.c

[Joel Brobecker]

> > I'll create the GDB PR but all explanations of the issue have been
> > provided on the mailing list by gdb developers and maintainers.
> > 
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=23663

Thanks everyone. the PR number is perfect, as it provides a nice
entry point to the everything related to this issue if anyone needs
to investigate it.

-- 
Joel