From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 9735C385741D for ; Mon, 25 Oct 2021 18:50:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9735C385741D Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 19PInx3U029189 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 25 Oct 2021 14:50:04 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 19PInx3U029189 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id A09B11ECEB; Mon, 25 Oct 2021 14:49:59 -0400 (EDT) Message-ID: Date: Mon, 25 Oct 2021 14:49:59 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.1 Subject: Re: [PATCH 1/2] gdbsupport: add assertions in array_view Content-Language: en-US To: Tom Tromey , Simon Marchi via Gdb-patches Cc: Simon Marchi References: <20211020010944.18780-1-simon.marchi@polymtl.ca> <8735opq7da.fsf@tromey.com> From: Simon Marchi In-Reply-To: <8735opq7da.fsf@tromey.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 25 Oct 2021 18:49:59 +0000 X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Oct 2021 18:50:08 -0000 On 2021-10-25 14:36, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches writes: > > Simon> From: Simon Marchi > Simon> Add assertions to ensure we don't access an array_view out of bounds. > Simon> Enable these assertions only when _GLIBCXX_DEBUG is set, as we did for > Simon> gdb::optional. > > Seems like a good idea to me. Ok, thanks, I'll push. > > Simon> - { return m_array[index]; } > Simon> + { > Simon> +#if defined(_GLIBCXX_DEBUG) > Simon> + gdb_assert (index < m_size); > Simon> +#endif > Simon> + return m_array[index]; > > I wonder if I ought to change the new type::field assert to be > conditional like this. Do we have some rule about which ones should be > conditional on _GLIBCXX_DEBUG? I don't know how we could make an objective rule for this... we would need to real-world performance measurements. Here, I was thinking that accessing each element of potentially large arrays (like, gdb_byte arrays, for example) would be done like this: for (int i = 0; i < view.size (); ++i) // access view[i] Adding a condition for each iteration sounds a bit wasteful... but I have no data to back this up. Also, array_view isn't that much used throughout the code yet. As it creeps a bit more everywhere, it's possible that performance hits due to this assert are only introduced later. We could also make the argument that since these branches are never taken, they should be predicted fairly well, so incur almost no cost. We could even tag the condition in gdb_assert with __builtin_expect to tell the compiler to expect that this branch won't be taken. There is other impacts of having the check than branch prediction though, like code density. I remember you have removed some assertions in the DWARF attributes code, because they were slowing things down. I'd be curious to see if we'd see any difference if we re-introduce them but use __builtin_expect. For type::field, I think that type::field uses are not frequent enough that it would make a difference. So having the assert always there would be worthwhile. > Or maybe type::fields should return an array view... Seems like a good idea regardless. Simon