From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id B72EB3858D1E for ; Wed, 11 Oct 2023 13:50:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B72EB3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1697032233; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=8y6Oj04mLpd1wZ6eWSzPzOyrpiYeIwqxZjvFEsFdJMk=; b=f23cVp3wIXcYferk16aS+rTrhjoMk4u8b/M5lwYmJiKagLAIJWIsuDYPsE7qpnuP8lujRi raBYtWHfWaLGV4eTgBC1Bf/NBJ1pqAS3DAFufOnGqgSX1bUI/nr1ZJDgfLTOJSIFon20HB V5PpWh4lHkqeeEEMN+L0FX9CVRynD2w= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-81-TcVbm5plPq6ADFcnZYCIMw-1; Wed, 11 Oct 2023 09:50:29 -0400 X-MC-Unique: TcVbm5plPq6ADFcnZYCIMw-1 Received: by mail-ed1-f70.google.com with SMTP id 4fb4d7f45d1cf-5344aaf2703so5530685a12.0 for ; Wed, 11 Oct 2023 06:50:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697032228; x=1697637028; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=8y6Oj04mLpd1wZ6eWSzPzOyrpiYeIwqxZjvFEsFdJMk=; b=WJGed3HX8Gr0z/l66zimqsloG+MmrU2J9TxMUWOmSOFZnH5Z6mwC3p6O5tqhZWHLTd pfu6aihTa6jDhDpPdP9hqEf8gV/DN9RE+cEHzinIdh17e3yFZgLSqrX45kUhnrOzB0DS 223bL5RIz41CFawH16haD89zeFDFFkOxKA/5VLEtBu9D0kaCJNUvcO8UKSLOLw6PPsSD i5vW4Rgc2ib21ET7Dx2PyGOKVoEVWi1bFfjfxQqm8UgdmutNgPF45HjxNA5w1LPAmWAT BzNyF6VqkiU9j5oaiPHC2AmiQ/KmlGwugp6rHp3p/RFAvGfUEBy9Mv3g5rCVW7vYgA6c tPXQ== X-Gm-Message-State: AOJu0YxM6qMw/15+4PM+lA3SV7rLgV6iaFuDFIn9kRI0iedsMB06AwR6 iUFS4qHBvsDqmhJiPSO+sjhVnzbGQod7vHH/G02vrTFxhJjJ4kaIBltWZOzqpmF7KQolzgSKPNv R/o2Y5uXrONbgBCAAt4FnLMbYPA== X-Received: by 2002:a05:6402:20c:b0:532:bc4d:9076 with SMTP id t12-20020a056402020c00b00532bc4d9076mr17016633edv.19.1697032228497; Wed, 11 Oct 2023 06:50:28 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHDHUYidyIOLafTGiqaw0xfBxtTKgZN9U3lm+uJ05yCDoGprgYWYirRKQxuXJl0kLaURhYQkA== X-Received: by 2002:a05:6402:20c:b0:532:bc4d:9076 with SMTP id t12-20020a056402020c00b00532bc4d9076mr17016620edv.19.1697032228058; Wed, 11 Oct 2023 06:50:28 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id w15-20020aa7cb4f000000b0052a063e52b8sm8886813edt.83.2023.10.11.06.50.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 06:50:27 -0700 (PDT) From: Andrew Burgess To: Nick Clifton , binutils@sourceware.org Subject: Re: [PATCH] bfd: add new bfd_cache_size() function In-Reply-To: <6bd18d16-728f-88c8-c31d-83a9f594a752@redhat.com> References: <6bd18d16-728f-88c8-c31d-83a9f594a752@redhat.com> Date: Wed, 11 Oct 2023 14:50:26 +0100 Message-ID: <875y3dclp9.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Nick Clifton 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 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 <> 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;