From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 4G7oAo2DP2UGlToAWB0awg (envelope-from ) for ; Mon, 30 Oct 2023 06:21:01 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=ayAx3vIg; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id F02421E11B; Mon, 30 Oct 2023 06:21:00 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id DC53F1E0C1 for ; Mon, 30 Oct 2023 06:20:58 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5B692385842A for ; Mon, 30 Oct 2023 10:20:58 +0000 (GMT) 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 1BE1B3858404 for ; Mon, 30 Oct 2023 10:20:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1BE1B3858404 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 1BE1B3858404 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698661247; cv=none; b=vCFtBqCTpFyiVU+PqT5k6iBL3LMh8ShLv1L4s5Gu2BJJhe92rKdOx1HxuXHouZ6BD4cDCo4A6I9otUloaTkw9kvwU8OXGFb0iysb/3hqag+BxM77gsdk4S1pWQ3L0udSbTMYA1rmNN3/qqZEEJ4dtCrZJwNVQBfpAIOKoDlG4no= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698661247; c=relaxed/simple; bh=0/ShVARCZR0H+2KzFOUaJRVjrjU0ktmSZykfa+UogYg=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=Gm+r7Dd8qrLq5gtQoGuE0F8AVf9ds2EiAGP/Wa7q6quMQfpgXws3C9fuI2RoZFpUInJFrcVCRcXZRkzmo6OOk/BhAmoz1QYP1+O3nywvFdOLW6/iLvssFMz8i0/7/PV9yS4+EzAZ0D0/hzN+QmqTMwWsDxXffHyM4z8rdAA0ayk= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698661245; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=s5ToEUdtZ7Q2IEWFiEhKbIBlkXht8+Zzzkbj0sl1QtQ=; b=ayAx3vIg5Q3zdQGmrW6PCX/CNFArfnqq8ogZeRk++hkhi4dBf7Li3PtUmBb8x0hl1esPKh R1exyNm6y/Y/HF5cqz6tmGzL/D0IblHPrUlB+EPM+f+z1Bo15kmUuUWj4zX4PNeS3R5Lne E0wDWr6HdEKBFWJlSILcTqDwuZ8fVvY= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-187-V02dbVTtO1-sV-oYk5Wknw-1; Mon, 30 Oct 2023 06:20:44 -0400 X-MC-Unique: V02dbVTtO1-sV-oYk5Wknw-1 Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-9c983b42c3bso597228166b.1 for ; Mon, 30 Oct 2023 03:20:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698661243; x=1699266043; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=s5ToEUdtZ7Q2IEWFiEhKbIBlkXht8+Zzzkbj0sl1QtQ=; b=P+D4iQx0XUgoiJhLSI/nmaJUCCtIAa6ecaPhbyvnldW1q3m5UnMWapDXxVmw7psiNG C0osrO6WEgHBH1JBnH5ZB+VTZXtLhr5RjxfaoEk7GeDzz1pziD6p74O0rR6HVaIgkfeV Bn7lSb+pA6xVKkDJ8TyE6GtoIYmkjHL99Ik7tkVy2A+jKinAbIbdGDzNN+koY3Y95G86 t4LfZC0Cp9Hq6VDU7FzAZkPSg2bEbigfJ3jGij3NRve6JNc+pOCszFNeuy7u4tfD3jS+ 9GPZ82xz2TFeKWcRj0WftkQfvlJALgK7rM/UGuY2usFUAE0Piofm7+3En7WMa3JvDAUF +bUg== X-Gm-Message-State: AOJu0YykqgLQ16biN3EiHZXf1zEt46f8hAK+Xh/DYyOlHt1ot2d2qIAP XWwlS6xbix+SWjD3cJE4lCPO8c9u0Biz9/Gz7sRP9czQyVwIX7ctCHrGPRVH3NnFd+NePxguF8T wZYgUL7UK22X+C02ChJuPtQ== X-Received: by 2002:a17:907:96a3:b0:9a5:dc2b:6a5 with SMTP id hd35-20020a17090796a300b009a5dc2b06a5mr10446701ejc.35.1698661242957; Mon, 30 Oct 2023 03:20:42 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFI68omUKvQXpeRWmdEwXvP9lGhiDUmZm2R1yVrgjRCNF3y0Qbw7iUSEtGF/fHIn+fksvoVCw== X-Received: by 2002:a17:907:96a3:b0:9a5:dc2b:6a5 with SMTP id hd35-20020a17090796a300b009a5dc2b06a5mr10446686ejc.35.1698661242499; Mon, 30 Oct 2023 03:20:42 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id lz23-20020a170906fb1700b009a5f1d1564dsm5748077ejb.126.2023.10.30.03.20.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 03:20:42 -0700 (PDT) From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c In-Reply-To: <87o7gjlv6q.fsf@tromey.com> References: <87o7gjlv6q.fsf@tromey.com> Date: Mon, 30 Oct 2023 10:20:40 +0000 Message-ID: <87r0lc1kef.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=-5.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Tom Tromey writes: >>>>>> "Andrew" == Andrew Burgess writes: > > Andrew> Better I think would be to track down where the bfd is being opened > Andrew> and add a corresponding bfd_cache_close_all() call elsewhere in GDB > Andrew> once we've finished doing whatever it is that caused us to open the > Andrew> bfd in the first place. > > I had contemplated this solution as well, though using bfd_cache_close > on the specific BFD, instead. > > Like you point out, though, it seems finicky to get right. We could > move up the call chain and add the close calls at some higher level, but > even that seems hard to be assured of. > > Andrew> (a) Just before we display a GDB prompt. We display a prompt after > Andrew> completing a command, and GDB is about to enter an idle state > Andrew> waiting for further input from the user (or in async mode, for an > Andrew> inferior event). If while we are in this idle state the user > Andrew> changes the on-disk file(s) then we would like GDB to notice this > Andrew> the next time it leaves its idle state, e.g. the next time the user > Andrew> executes a command, or when an inferior event arrives, > > Andrew> (b) When we resume the inferior. In synchronous mode, resuming the > Andrew> inferior is another time when GDB is blocked and sitting idle, but > Andrew> in this case we don't display a prompt. As with (a) above, when an > Andrew> inferior event arrives we want GDB to notice any changes to on-disk > Andrew> files. > > I've been working on moving DWARF reading to the background. For the > most part, your plan won't be an issue -- gdb pre-reads the relevant > section data on the main thread and doesn't require the BFD to be open > when scanning. I did remember you mentioning your background debug parsing work, but I couldn't find any emails about it, but maybe we talked in IRC... > However, there is one case where this will change: opening dwo files. > Now, the way the background reading is implemented, the worst case here > is some inefficiency: the main thread might close all the BFDs, and then > the background reader might immediately reopen one. Right now I don't see bfd/cache.c as thread safe, so are you planning to make it thread safe? Or place restrictions within GDB to prevent multiple threads accessing BFDs at the same time (e.g. background reading can only happen when GDB itself is known to be idle)? > Maybe this isn't something to worry about? I could make the background > reader also call bfd_cache_close for the DWO BFDs, to ensure they are > closed. (This only really matters for the "rebuild" case, not any sort > of exec case, because DWOs aren't really part of the inferior.) I figured all I'm really doing is moving the bfd_cache_close_all() calls, we already had them randomly scattered throughout GDB anyway, so any background reader you implement would already need to deal with the cache being cleared under its feet (or prevent that from happening when its running), so I didn't think I'd made things harder for you ... just moved the problem around a bit. Also, I figured it's not that hard to implement a gdb_bfd_cache_close_all() wrapper which counts the number of threads that are actively using the cache, and only calls bfd_cache_close_all() if the "other" thread isn't currently running. Of course, that assumes that the cache itself is somehow thread-safe. Another option is to only perform background reading while GDB is in-active, in which case, my current bfd_cache_close_all() calls are actually the points at which your backgound threads can safely start working without worrying about GDB also accessing the bfd cache -- though, you'd still need a mechanism to stop the background reader once GDB becomes active again. The other option I considered, is that you could add a mechanism to allow BFDs to be opened without going through the BFD cache. In this case, I think your thread safety issues go away, and also, you no longer care about my bfd_cache_close_all() calls. The BFD cache exists to handle systems that only allow a small number of open files (they quote 20 as being a limit on some systems), so avoiding the cache might run into an open FD limit issue ... but I'm not sure if this low limit problem is really a thing on systems that are going to be running multiple threads? > > Andrew> Right now, the only other users of the observers that I'm connecting > Andrew> too are the extension languages, specifically, Python. I suspect it > Andrew> must be possible for Python to carry out actions that can cause the > Andrew> bfd cache to be populated, so maybe there is a risk here. > > Andrew> To counter this risk, we could move the bfd_cache_close_all() calls > Andrew> out of observers, and move them into GDB core close too, but after the > Andrew> two observers in questions are actually called, so into > Andrew> top_level_prompt for the before_prompt observer and into > Andrew> notify_target_resumed for the target_resumed observer. > > Andrew> Another choice would be to add a bfd_cache_close_all() into > Andrew> gdbpy_enter::~gdbpy_enter, so whenever we call into a Python > Andrew> extension, we always clear the bfd cache once we're done. > > Andrew> For now I haven't made either of these changes, maybe I'm worrying > Andrew> about nothing? I'd appreciate peoples thoughts. > > Do we know of specific Python APIs that could cause problems? Well the generic (maybe unhelpful) answer is gdb.execute(). > > I guess anything that reads memory if trust-readonly-sections is > enabled? That's the only one I could think of offhand. > > Maybe we need a combo approach where some calls call bfd_cache_close at > the end. I'd really prefer to avoid relying on folk to remember that they need to call bfd_cache_close_all() in certain places. I think its far too easy to forget, and these are bugs that are only going to crop up once in a while .... A user is running a particular Python extension, and performs a particular action, just at the same time as they recompile their test binary ... and suddenly GDB "misses" that the test executable changed. Bugs like this can be a nightmare to track down. Right now, I'm thinking I'll switch away from using the observers, and just ensure that bfd_cache_close_all() is called after all the observers have run. Thanks, Andrew