[PATCH] bfd: add new bfd_cache_size() function

[PATCH] bfd: add new bfd_cache_size() function

[Andrew Burgess]

In GDB we have a problem with the BFD cache.

As GDB runs for a potentially extended period of time, if the BFD
cache holds a file descriptor for an open on-disk file, this can, on
some targets (e.g. Win32) prevent the OS writing to the file.

This might, for example, prevent a user from recompiling their
executable as GDB is (via the BFD cache) holding an open reference to
that file.

Another problem, relates to bfd_stat, for BFDs that are using the BFD
cache (i.e. they call cache_bstat to implement bfd_stat).  The
cache_bstat function finds the BFD in the cache, opening the file if
needed, and then uses fstat on the open file descriptor.

What this means is that, if the on-disk file changes, but the cache
was holding an open reference to the file, the bfd_stat will return
the 'struct stat' for the old file, not the new file.

Now, for this second problem, we might be tempted to make use of an
actual stat call, instead of calling bfd_stat, however, this isn't
ideal as we have some BFDs that use a custom iovec, and implement the
various functions over GDB's remote protocol.  By using bfd_stat we
can have a single call that should work for both local files, and for
remote files.

To solve both of these problems GDB has calls to bfd_cache_close_all
sprinkled around its code base.  And in theory this should work fine.

However, I recently ran into a case where we had missed a
bfd_cache_close_all call, and as a result some BFDs were held open.
This caused a bfd_stat call to return an unexpected result (old file
vs new file).

What I'd like is some way within GDB that I can do:

  gdb_assert ( /* Nothing is held open in the cache.  */ );

As this would allow GDB to quickly identify when we've missed some
bfd_cache_close_all calls.

And so, to support this, I would like to add a new bfd_cache_size
function.  This function returns an integer, which is the number of
open files in the cache.  I can then start adding:

  gdb_assert (bfd_cache_size() == 0);

to GDB in some strategic spots, and start fixing all of the missing
bfd_cache_close_all calls that crop up as a result.
---
 bfd/bfd-in2.h |  2 ++
 bfd/cache.c   | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index eddb9902f5e..8ac7a2f2f8e 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -2790,6 +2790,8 @@ bool bfd_cache_close (bfd *abfd);
 
 bool bfd_cache_close_all (void);
 
+int bfd_cache_size (void);
+
 /* Extracted from compress.c.  */
 /* Types of compressed DWARF debug sections.  */
 enum compressed_debug_section_type
diff --git a/bfd/cache.c b/bfd/cache.c
index 357a38da599..8db42a26e66 100644
--- a/bfd/cache.c
+++ b/bfd/cache.c
@@ -574,6 +574,23 @@ bfd_cache_close_all (void)
   return ret;
 }
 
+/*
+FUNCTION
+	bfd_cache_size
+
+SYNOPSIS
+	int bfd_cache_size (void);
+
+DESCRIPTION
+	Return the number of open files in the cache.
+*/
+
+int
+bfd_cache_size (void)
+{
+  return open_files;
+}
+
 /*
 INTERNAL_FUNCTION
 	bfd_open_file

base-commit: 9a896be33224654760c46d3698218241d0a1f354
-- 
2.25.4

Re: [PATCH] bfd: add new bfd_cache_size() function

[Torbjorn SVENSSON]

On 2023-10-06 18:03, Andrew Burgess via Binutils wrote:
> In GDB we have a problem with the BFD cache.
> 
> As GDB runs for a potentially extended period of time, if the BFD
> cache holds a file descriptor for an open on-disk file, this can, on
> some targets (e.g. Win32) prevent the OS writing to the file.
> 
> This might, for example, prevent a user from recompiling their
> executable as GDB is (via the BFD cache) holding an open reference to
> that file.
> 
> Another problem, relates to bfd_stat, for BFDs that are using the BFD
> cache (i.e. they call cache_bstat to implement bfd_stat).  The
> cache_bstat function finds the BFD in the cache, opening the file if
> needed, and then uses fstat on the open file descriptor.
> 
> What this means is that, if the on-disk file changes, but the cache
> was holding an open reference to the file, the bfd_stat will return
> the 'struct stat' for the old file, not the new file.
> 
> Now, for this second problem, we might be tempted to make use of an
> actual stat call, instead of calling bfd_stat, however, this isn't
> ideal as we have some BFDs that use a custom iovec, and implement the
> various functions over GDB's remote protocol.  By using bfd_stat we
> can have a single call that should work for both local files, and for
> remote files.
> 
> To solve both of these problems GDB has calls to bfd_cache_close_all
> sprinkled around its code base.  And in theory this should work fine.
> 
> However, I recently ran into a case where we had missed a
> bfd_cache_close_all call, and as a result some BFDs were held open.
> This caused a bfd_stat call to return an unexpected result (old file
> vs new file).
> 
> What I'd like is some way within GDB that I can do:
> 
>    gdb_assert ( /* Nothing is held open in the cache.  */ );
> 
> As this would allow GDB to quickly identify when we've missed some
> bfd_cache_close_all calls.
> 
> And so, to support this, I would like to add a new bfd_cache_size
> function.  This function returns an integer, which is the number of
> open files in the cache.  I can then start adding:
> 
>    gdb_assert (bfd_cache_size() == 0);
> 
> to GDB in some strategic spots, and start fixing all of the missing
> bfd_cache_close_all calls that crop up as a result.

I think this sounds reasonable, but I'm not a maintainer so you need 
someone else to approve this.

Re: [PATCH] bfd: add new bfd_cache_size() function

[Nick Clifton]

Hi Andrew,

> And so, to support this, I would like to add a new bfd_cache_size
> function.  This function returns an integer, which is the number of
> open files in the cache.  I can then start adding:
> 
>    gdb_assert (bfd_cache_size() == 0);
> 
> to GDB in some strategic spots, and start fixing all of the missing
> bfd_cache_close_all calls that crop up as a result.

Patch approved - please apply.

If you are feeling motivated then there is an associated change
that it would be nice to see made:

> +/*
> +FUNCTION
> +	bfd_cache_size
> +
> +SYNOPSIS
> +	int bfd_cache_size (void);
> +
> +DESCRIPTION
> +	Return the number of open files in the cache.
> +*/

It does not make sense for this function to return a negative
value.  (Or maybe it does - a negative value would indicate that
the cache does not exist, whereas 0 would indicate that it does
exist, but it is empty ?).

So if bfd_cache_size() returns an unsigned int then bfd_cache_max_open()
should as well, and the files_open and max_files_open variables should
be changed as well.

Of course in practice we should never see negative values or large values
for any of these variables/function-results, so using an "int" should be
just fine.  But it bugs me that functions and variables which should never
have negative values are being typed as if they could have them.

Cheers
   Nick

Re: [PATCH] bfd: add new bfd_cache_size() function

[Andrew Burgess]

Nick Clifton <nickc@redhat.com> writes:

> Hi Andrew,
>
>> And so, to support this, I would like to add a new bfd_cache_size
>> function.  This function returns an integer, which is the number of
>> open files in the cache.  I can then start adding:
>> 
>>    gdb_assert (bfd_cache_size() == 0);
>> 
>> to GDB in some strategic spots, and start fixing all of the missing
>> bfd_cache_close_all calls that crop up as a result.
>
> Patch approved - please apply.
>
> If you are feeling motivated then there is an associated change
> that it would be nice to see made:
>
>> +/*
>> +FUNCTION
>> +	bfd_cache_size
>> +
>> +SYNOPSIS
>> +	int bfd_cache_size (void);
>> +
>> +DESCRIPTION
>> +	Return the number of open files in the cache.
>> +*/
>
> It does not make sense for this function to return a negative
> value.  (Or maybe it does - a negative value would indicate that
> the cache does not exist, whereas 0 would indicate that it does
> exist, but it is empty ?).
>
> So if bfd_cache_size() returns an unsigned int then bfd_cache_max_open()
> should as well, and the files_open and max_files_open variables should
> be changed as well.
>
> Of course in practice we should never see negative values or large values
> for any of these variables/function-results, so using an "int" should be
> just fine.  But it bugs me that functions and variables which should never
> have negative values are being typed as if they could have them.

Ask and you shall receive!

How's the patch below?  This applies onto current HEAD without my
bfd_cache_size patch and makes the int -> unsigned changes you suggest.

I would then update my bfd_cache_size patch to return unsigned -- but
I'll just go ahead and merge the updated version assuming this patch
here is approved.

Thanks,
Andrew

---

commit 51393c2fcaf8d6d1f8d23d6e4caecf13e40e3279
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Oct 11 14:39:37 2023 +0100

    bfd/cache: change type used to track cached BFDs from int to unsigned
    
    Within bfd/cache.c change the type for max_open_files and open_files
    variables from int to unsigned.  As a consequence of this, the return
    type for bfd_cache_max_open() is also changed from int to unsigned.
    
    Within bfd_cache_max_open I've left the local 'max' variable as an
    int, this should ensure that if the sysconf call fails, and returns
    -1, then the computed max value will be less than 10, which means
    max_open_files will be set to 10.  If 'max' was changed to unsigned
    then, should the sysconf call fail, we'd end up with max becoming a
    very large positive number ... which is clearly not what we want.
    
    And, while I was auditing how open_files is used, I added an assert
    within bfd_cache_delete to ensure that we don't try to reduce
    open_files below zero.
    
    There should be no user visible change with this commit.

diff --git a/bfd/cache.c b/bfd/cache.c
index 357a38da599..87c4bcd3148 100644
--- a/bfd/cache.c
+++ b/bfd/cache.c
@@ -67,11 +67,11 @@ enum cache_flag {
 /* The maximum number of files which the cache will keep open at
    one time.  When needed call bfd_cache_max_open to initialize.  */
 
-static int max_open_files = 0;
+static unsigned max_open_files = 0;
 
 /* Set max_open_files, if not already set, to 12.5% of the allowed open
    file descriptors, but at least 10, and return the value.  */
-static int
+static unsigned
 bfd_cache_max_open (void)
 {
   if (max_open_files == 0)
@@ -115,7 +115,7 @@ bfd_cache_max_open (void)
 
 /* The number of BFD files we have open.  */
 
-static int open_files;
+static unsigned open_files;
 
 /* Zero, or a pointer to the topmost BFD on the chain.  This is
    used by the <<bfd_cache_lookup>> macro in @file{libbfd.h} to
@@ -176,6 +176,7 @@ bfd_cache_delete (bfd *abfd)
   snip (abfd);
 
   abfd->iostream = NULL;
+  BFD_ASSERT (open_files > 0);
   --open_files;
   abfd->flags |= BFD_CLOSED_BY_CACHE;
 

Re: [PATCH] bfd: add new bfd_cache_size() function

[Nick Clifton]

Hi Andrew,

>> So if bfd_cache_size() returns an unsigned int then bfd_cache_max_open()
>> should as well, and the files_open and max_files_open variables should
>> be changed as well.
>>
>> Of course in practice we should never see negative values or large values
>> for any of these variables/function-results, so using an "int" should be
>> just fine.  But it bugs me that functions and variables which should never
>> have negative values are being typed as if they could have them.
> 
> Ask and you shall receive!
> 
> How's the patch below?  This applies onto current HEAD without my
> bfd_cache_size patch and makes the int -> unsigned changes you suggest.

Excellent - that is exactly what I had in mind.

> I would then update my bfd_cache_size patch to return unsigned -- but
> I'll just go ahead and merge the updated version assuming this patch
> here is approved.

It is.  Please go ahead and check in both changes.

Cheers
   Nick

Re: [PATCH] bfd: add new bfd_cache_size() function

[Andrew Burgess]

Nick Clifton <nickc@redhat.com> writes:

> Hi Andrew,
>
>>> So if bfd_cache_size() returns an unsigned int then bfd_cache_max_open()
>>> should as well, and the files_open and max_files_open variables should
>>> be changed as well.
>>>
>>> Of course in practice we should never see negative values or large values
>>> for any of these variables/function-results, so using an "int" should be
>>> just fine.  But it bugs me that functions and variables which should never
>>> have negative values are being typed as if they could have them.
>> 
>> Ask and you shall receive!
>> 
>> How's the patch below?  This applies onto current HEAD without my
>> bfd_cache_size patch and makes the int -> unsigned changes you suggest.
>
> Excellent - that is exactly what I had in mind.
>
>> I would then update my bfd_cache_size patch to return unsigned -- but
>> I'll just go ahead and merge the updated version assuming this patch
>> here is approved.
>
> It is.  Please go ahead and check in both changes.

Both changes pushed.

Thanks,
Andrew