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 AA0C03858CDA for ; Mon, 5 Sep 2022 12:54:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AA0C03858CDA 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=1662382463; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=euUnVNaURkchM37Gx1NMZYglhjEqX7CHSv8UBKSOLEQ=; b=TW3YlRtIPIwfWBQNU8WrOBj+I8e8EK5PW82uQBGpBPbnJXvIobcJixTOXYnO+OwuUbnb/z LTGn4sd/gYSaaOoU5DIhfU5iWgG11w1uKzQ69m3WUtk8SeLDPrCYEWpKgH0J5yAAyFsk0A AvglpemsVx4j+O1G7Yo5a23JAohiJn4= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-201-CbOxNFY6MyOtjEeuBdYToQ-1; Mon, 05 Sep 2022 08:54:14 -0400 X-MC-Unique: CbOxNFY6MyOtjEeuBdYToQ-1 Received: by mail-wm1-f71.google.com with SMTP id n7-20020a1c2707000000b003a638356355so5353655wmn.2 for ; Mon, 05 Sep 2022 05:54:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:subject:from:references:to :content-language:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=euUnVNaURkchM37Gx1NMZYglhjEqX7CHSv8UBKSOLEQ=; b=RL3PCvhS1khngpycjy/9qmyZzDF9Qb7KqpwSgDGWh1TtLnD3qsSoQPEeZbq5cqNR4s XI4CV3x3rpVjuGpjQNcSwQQPRzdFdYdiUQE4IUJaVrTPHvkVv2xSUc7ZFCZYR0o9BBLH Sw9rgzDKbxNps/5iHQuPLuIkOcUUA9tIxdPD9w91gYubi3XuH7vLDF4hr7CeJmnCfpPT fIgtQQRohpKGX+x6oy6tpHeq83A9K5XVJ+4SCYSp+dl86XnKi0c9cFhugInIqsU7o6/p hH9Wggnk/MhuOy9JYd1jciq3hO9DP9M8OdrPBwFc6zRxmfeCiSBjAFhzI6KfIIpBhKqo vTMw== X-Gm-Message-State: ACgBeo1d/a4xTNdf56Z8ffQjmnrts4TBKHN1il5HPyFO0SB56IvrVciI dXu8gad/yo8h0eExt4m0WBfn8gvTRFLDvkVqfvdCeBTSrkjrE+EnbPstEWi+AW+zcP0qFFlKsTo pyOYaS7DUC1NKwN+5YQ== X-Received: by 2002:a05:600c:444b:b0:3a6:6b99:239a with SMTP id v11-20020a05600c444b00b003a66b99239amr10916313wmn.41.1662382453205; Mon, 05 Sep 2022 05:54:13 -0700 (PDT) X-Google-Smtp-Source: AA6agR5/xFOkwj85wJzAZ2qDy1ndjE1VDe6J71MKvmhsBx+fy0Gr5I8hvy2jtEWOihsGZDTmDceMog== X-Received: by 2002:a05:600c:444b:b0:3a6:6b99:239a with SMTP id v11-20020a05600c444b00b003a66b99239amr10916296wmn.41.1662382452877; Mon, 05 Sep 2022 05:54:12 -0700 (PDT) Received: from [192.168.1.6] (adsl-2-solo-236-177.claranet.co.uk. [80.168.236.177]) by smtp.gmail.com with ESMTPSA id w4-20020adfee44000000b0022863c18b93sm4910898wro.13.2022.09.05.05.54.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 05 Sep 2022 05:54:12 -0700 (PDT) Message-ID: <174963e9-fa71-b70e-7659-0f534db073f8@redhat.com> Date: Mon, 5 Sep 2022 13:54:11 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 To: =?UTF-8?Q?Martin_Storsj=c3=b6?= , binutils@sourceware.org References: <20220902105903.2249507-1-martin@martin.st> From: Nick Clifton Subject: Re: [PATCH] ld: pe: Improve performance of object file exclude symbol directives In-Reply-To: <20220902105903.2249507-1-martin@martin.st> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-GB Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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: Hi Martin, > Store the list of excluded symbols in a sorted list, speeding up > checking for duplicates when inserting new entries. An excellent idea. > /* From EXCLUDE_SYMBOLS or embedded directives. */ > + int num_exclude_symbols; Presumably there can never be a negative number of excluded symbols, so using "unsigned int" here might be more appropriate. > + if (fdef->exclude_symbols) Given that the for() loop to follow checks fdef->num_exclude_symbols, this if() statement is redundant. > + for (i = 0; i < fdef->num_exclude_symbols; i++) > + free (fdef->exclude_symbols[i].symbol_name); > + free (fdef->exclude_symbols); Calling free(NULL) is allowed, so the if() statement is still not needed. > +/* Search the position of the identical element, or returns the position > + of the next higher element. If last valid element is smaller, then MAX > + is returned. */ The comment should explain the use of the IS_IDENT parameter. It should probably also note that MAX is expected to be the same as the number of entries in the B array. > +static int > +find_exclude_in_list (def_file_exclude_symbol *b, int max, > + const char *name, int *is_ident) Since we are dealing with array indicies here, I would suggest that the function should return an unsigned int. Likewise the MAX parameter should be unsigned. Plus is_ident should be a boolean pointer, not an integer pointer. > +static def_file_exclude_symbol * > +def_file_add_exclude_symbol (def_file *fdef, const char *name, int *is_dup) I think that IS_DUP should be a "boolean *" not a "int *". > + int max_exclude_symbols = ROUND_UP(fdef->num_exclude_symbols, 32); Magic numbers are bad. Please replace 32 with a defined constant. > + if (fdef->num_exclude_symbols >= max_exclude_symbols) This is sneaky and not very intuitive. I would prefer it if you had two fields in the fdef structure, one for the number of allocated slots in the exclude_symbols array and one for the number of used slots. Then testing for when the array needs to be extended would be a case of comparing these two fields. Using two fields might take up more space in the structure, but it sure makes a lot more sense to me. > + max_exclude_symbols = ROUND_UP(fdef->num_exclude_symbols + 1, 32); Given that the point of this patch is to improve performance when there are a large number of excluded symbols, incrementing the array by 32 slots at a time seems counter intuitive. I would suggest a bigger number, eg 1024 or 10240. > + e = fdef->exclude_symbols + pos; > + if (pos != fdef->num_exclude_symbols) > + memmove (&e[1], e, (sizeof (def_file_exclude_symbol) * (fdef->num_exclude_symbols - pos))); What the ? OK, so what is going on here ? At the very least I think that you are going to need to add a comment to this piece of code. Cheers Nick Cheers Nick