From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com [IPv6:2a00:1450:4864:20::22d]) by sourceware.org (Postfix) with ESMTPS id DE1683851403 for ; Tue, 6 Sep 2022 15:44:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DE1683851403 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=martin.st Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=martin.st Received: by mail-lj1-x22d.google.com with SMTP id c10so3270669ljj.2 for ; Tue, 06 Sep 2022 08:44:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=martin-st.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:from:to:cc:subject:date; bh=S3LV7CnyAox4vuFcocAxpO7BcrK2D0iFsEm5w/LpPSQ=; b=X58g5i2x85ZpugfHgpdW6MCvO6dCswFPbH70P81qWaXkM6/hknY2Ah2yPcFavRhLBJ DLgiSXxEOQj+rjllqsjUTdp2QL6lYcE2yV0m5E6Kye1i3GDMK3EuoJw5nX6ydrA34YI5 /IWRVxh9q4YEpHRAQLfj/6UgiZlzFBuArwtDr4fiwR5U+JhlXhOavlMcMf7rqArlpvrM neGkY5yKA4xEiV8YCSnbFCGqQVHstdtPfbSyzqUL9CCaBbyqI0eJD1Zmt6kitsf9Z4sG xJ0jTr5uG7B8rN775LpAvQaots9Hnt6LTH6Ryaj+FtjcZSoKQp1Z7iZwOJsiRwI8iNPR 0ugA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date; bh=S3LV7CnyAox4vuFcocAxpO7BcrK2D0iFsEm5w/LpPSQ=; b=axE+XE9ADJCA8WWOz/rQWkpBlsYXYYLPagODmlNVWl9pbgarZ42b31kYaxboDXVtJG viHSXn/AypfjdRX6UGdeuUEy0jHLqrm40PS1T6uMveaMmfO1K2zDFZK90uiMJs7t6ovs UI6Og70dw39mu70MVZ595fjOlvM7UNvNRm1DBu0n6IpJZqz9O+bcYNA3QUOttP46eiIX Gg/tYtydSLUzforFWHXmCbNp0fIVJm344xI4mrFNgQxy16ZQE7GGxWIS72fPYoUOd1i2 X9t1fbvNkUBXnwJQPZIjAl6HzMlVXtvhT0QGKgzzt/ghHSA2fKFI6C437PY5kWu11hno Db0A== X-Gm-Message-State: ACgBeo1Ms7LHwZHLioejCV//6vmPtCGl6xpdXqHDvjgQVV0KV1RQ83HI zc7LtxsdkLMfGIT9bsGA8DsI+XyEUiUUzDZ4 X-Google-Smtp-Source: AA6agR4EgFfjaCGERhpb6Dic/aDsYxCrV8w72GNbIYJ4ml00o09+c86qhPZ65r+TmY4lW0ZgfGXK2g== X-Received: by 2002:a2e:b5cd:0:b0:265:86d2:60a8 with SMTP id g13-20020a2eb5cd000000b0026586d260a8mr11738339ljn.50.1662479089396; Tue, 06 Sep 2022 08:44:49 -0700 (PDT) Received: from localhost.localdomain (dsl-tkubng21-58c01c-243.dhcp.inet.fi. [88.192.28.243]) by smtp.gmail.com with ESMTPSA id e2-20020ac25462000000b004949f7cbb6esm1809398lfn.79.2022.09.06.08.44.48 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Sep 2022 08:44:48 -0700 (PDT) From: =?UTF-8?q?Martin=20Storsj=C3=B6?= To: binutils@sourceware.org Subject: [PATCH v2 2/2] ld: pe: Apply review suggestions on the existing exports/imports arrays Date: Tue, 6 Sep 2022 18:44:47 +0300 Message-Id: <20220906154447.1402361-2-martin@martin.st> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20220906154447.1402361-1-martin@martin.st> References: <20220906154447.1402361-1-martin@martin.st> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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: Use a separate explicit max_exports/imports field, instead of deducing it from the number of allocated elements. Use a named constant for the incremental growth of the array. Use bool instead of int for boolean values. Remove an unnecessary if statement/scope in the def_file_free function. Add more verbose comments about parameters, and about insertion into an array of structs. Generally use unsigned integers for all array indices and sizes. The num_exports/imports fields are kept as is as signed integers, since changing them to unsigned would require a disproportionate amount of changes ti pe-dll.c to avoid comparisons between signed and unsigned. Simply use xrealloc instead of a check and xmalloc/xrealloc; xrealloc can take NULL as the first parameter (and does a similar check internally). (This wasn't requested in review though, but noticed while working on the code.) --- ld/deffile.h | 6 +- ld/deffilep.y | 157 ++++++++++++++++++++++++-------------------------- ld/pe-dll.c | 6 +- 3 files changed, 83 insertions(+), 86 deletions(-) diff --git a/ld/deffile.h b/ld/deffile.h index 89ffe9f972e..9eb08ad83fb 100644 --- a/ld/deffile.h +++ b/ld/deffile.h @@ -84,6 +84,7 @@ typedef struct def_file { /* From the EXPORTS commands. */ int num_exports; + unsigned int max_exports; def_file_export *exports; /* Used by imports for module names. */ @@ -91,6 +92,7 @@ typedef struct def_file { /* From the IMPORTS commands. */ int num_imports; + unsigned int max_imports; def_file_import *imports; /* From the VERSION command, -1 if not specified. */ @@ -112,10 +114,10 @@ extern def_file *def_file_parse (const char *, def_file *); extern void def_file_free (def_file *); extern def_file_export *def_file_add_export (def_file *, const char *, const char *, int, - const char *, int *); + const char *, bool *); extern def_file_import *def_file_add_import (def_file *, const char *, const char *, int, const char *, - const char *, int *); + const char *, bool *); extern int def_file_add_import_from (def_file *fdef, int num_imports, const char *name, diff --git a/ld/deffilep.y b/ld/deffilep.y index ed8f0d6719a..d7052a63beb 100644 --- a/ld/deffilep.y +++ b/ld/deffilep.y @@ -459,29 +459,23 @@ def_file_free (def_file *fdef) free (fdef->section_defs); } - if (fdef->exports) + for (i = 0; i < fdef->num_exports; i++) { - for (i = 0; i < fdef->num_exports; i++) - { - if (fdef->exports[i].internal_name != fdef->exports[i].name) - free (fdef->exports[i].internal_name); - free (fdef->exports[i].name); - free (fdef->exports[i].its_name); - } - free (fdef->exports); + if (fdef->exports[i].internal_name != fdef->exports[i].name) + free (fdef->exports[i].internal_name); + free (fdef->exports[i].name); + free (fdef->exports[i].its_name); } + free (fdef->exports); - if (fdef->imports) + for (i = 0; i < fdef->num_imports; i++) { - for (i = 0; i < fdef->num_imports; i++) - { - if (fdef->imports[i].internal_name != fdef->imports[i].name) - free (fdef->imports[i].internal_name); - free (fdef->imports[i].name); - free (fdef->imports[i].its_name); - } - free (fdef->imports); + if (fdef->imports[i].internal_name != fdef->imports[i].name) + free (fdef->imports[i].internal_name); + free (fdef->imports[i].name); + free (fdef->imports[i].its_name); } + free (fdef->imports); while (fdef->modules) { @@ -627,22 +621,25 @@ cmp_export_elem (const def_file_export *e, const char *ex_name, /* 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. */ + is returned. The max parameter indicates the number of elements in the + array. On return, *is_ident indicates whether the returned array index + points at an element which is identical to the one searched for. */ -static int -find_export_in_list (def_file_export *b, int max, +static unsigned int +find_export_in_list (def_file_export *b, unsigned int max, const char *ex_name, const char *in_name, - const char *its_name, int ord, int *is_ident) + const char *its_name, int ord, bool *is_ident) { - int e, l, r, p; + int e; + unsigned int l, r, p; - *is_ident = 0; + *is_ident = false; if (!max) return 0; if ((e = cmp_export_elem (b, ex_name, in_name, its_name, ord)) <= 0) { if (!e) - *is_ident = 1; + *is_ident = true; return 0; } if (max == 1) @@ -652,7 +649,7 @@ find_export_in_list (def_file_export *b, int max, else if (!e || max == 2) { if (!e) - *is_ident = 1; + *is_ident = true; return max - 1; } l = 0; r = max - 1; @@ -662,7 +659,7 @@ find_export_in_list (def_file_export *b, int max, e = cmp_export_elem (b + p, ex_name, in_name, its_name, ord); if (!e) { - *is_ident = 1; + *is_ident = true; return p; } else if (e < 0) @@ -673,7 +670,7 @@ find_export_in_list (def_file_export *b, int max, if ((e = cmp_export_elem (b + l, ex_name, in_name, its_name, ord)) > 0) ++l; else if (!e) - *is_ident = 1; + *is_ident = true; return l; } @@ -683,11 +680,10 @@ def_file_add_export (def_file *fdef, const char *internal_name, int ordinal, const char *its_name, - int *is_dup) + bool *is_dup) { def_file_export *e; - int pos; - int max_exports = ROUND_UP(fdef->num_exports, 32); + unsigned int pos; if (internal_name && !external_name) external_name = internal_name; @@ -695,27 +691,27 @@ def_file_add_export (def_file *fdef, internal_name = external_name; /* We need to avoid duplicates. */ - *is_dup = 0; + *is_dup = false; pos = find_export_in_list (fdef->exports, fdef->num_exports, external_name, internal_name, its_name, ordinal, is_dup); - if (*is_dup != 0) + if (*is_dup) return (fdef->exports + pos); - if (fdef->num_exports >= max_exports) + if ((unsigned)fdef->num_exports >= fdef->max_exports) { - max_exports = ROUND_UP(fdef->num_exports + 1, 32); - if (fdef->exports) - fdef->exports = xrealloc (fdef->exports, - max_exports * sizeof (def_file_export)); - else - fdef->exports = xmalloc (max_exports * sizeof (def_file_export)); + fdef->max_exports += SYMBOL_LIST_ARRAY_GROW; + fdef->exports = xrealloc (fdef->exports, + fdef->max_exports * sizeof (def_file_export)); } e = fdef->exports + pos; - if (pos != fdef->num_exports) + /* If we're inserting in the middle of the array, we need to move the + following elements forward. */ + if (pos != (unsigned)fdef->num_exports) memmove (&e[1], e, (sizeof (def_file_export) * (fdef->num_exports - pos))); + /* Wipe the element for use as a new entry. */ memset (e, 0, sizeof (def_file_export)); e->name = xstrdup (external_name); e->internal_name = xstrdup (internal_name); @@ -772,22 +768,25 @@ cmp_import_elem (const def_file_import *e, const char *ex_name, /* 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. */ + is returned. The max parameter indicates the number of elements in the + array. On return, *is_ident indicates whether the returned array index + points at an element which is identical to the one searched for. */ -static int -find_import_in_list (def_file_import *b, int max, +static unsigned int +find_import_in_list (def_file_import *b, unsigned int max, const char *ex_name, const char *in_name, - const char *module, int ord, int *is_ident) + const char *module, int ord, bool *is_ident) { - int e, l, r, p; + int e; + unsigned int l, r, p; - *is_ident = 0; + *is_ident = false; if (!max) return 0; if ((e = cmp_import_elem (b, ex_name, in_name, module, ord)) <= 0) { if (!e) - *is_ident = 1; + *is_ident = true; return 0; } if (max == 1) @@ -797,7 +796,7 @@ find_import_in_list (def_file_import *b, int max, else if (!e || max == 2) { if (!e) - *is_ident = 1; + *is_ident = true; return max - 1; } l = 0; r = max - 1; @@ -807,7 +806,7 @@ find_import_in_list (def_file_import *b, int max, e = cmp_import_elem (b + p, ex_name, in_name, module, ord); if (!e) { - *is_ident = 1; + *is_ident = true; return p; } else if (e < 0) @@ -818,7 +817,7 @@ find_import_in_list (def_file_import *b, int max, if ((e = cmp_import_elem (b + l, ex_name, in_name, module, ord)) > 0) ++l; else if (!e) - *is_ident = 1; + *is_ident = true; return l; } @@ -849,33 +848,30 @@ def_file_add_import (def_file *fdef, int ordinal, const char *internal_name, const char *its_name, - int *is_dup) + bool *is_dup) { def_file_import *i; - int pos; - int max_imports = ROUND_UP (fdef->num_imports, 16); + unsigned int pos; /* We need to avoid here duplicates. */ - *is_dup = 0; + *is_dup = false; pos = find_import_in_list (fdef->imports, fdef->num_imports, name, (!internal_name ? name : internal_name), module, ordinal, is_dup); - if (*is_dup != 0) + if (*is_dup) return fdef->imports + pos; - if (fdef->num_imports >= max_imports) + if ((unsigned)fdef->num_imports >= fdef->max_imports) { - max_imports = ROUND_UP (fdef->num_imports+1, 16); - - if (fdef->imports) - fdef->imports = xrealloc (fdef->imports, - max_imports * sizeof (def_file_import)); - else - fdef->imports = xmalloc (max_imports * sizeof (def_file_import)); + fdef->max_imports += SYMBOL_LIST_ARRAY_GROW; + fdef->imports = xrealloc (fdef->imports, + fdef->max_imports * sizeof (def_file_import)); } i = fdef->imports + pos; - if (pos != fdef->num_imports) + /* If we're inserting in the middle of the array, we need to move the + following elements forward. */ + if (pos != (unsigned)fdef->num_imports) memmove (i + 1, i, sizeof (def_file_import) * (fdef->num_imports - pos)); fill_in_import (i, name, def_stash_module (fdef, module), ordinal, @@ -895,36 +891,35 @@ def_file_add_import_from (def_file *fdef, const char *its_name ATTRIBUTE_UNUSED) { def_file_import *i; - int is_dup; - int pos; - int max_imports = ROUND_UP (fdef->num_imports, 16); + bool is_dup; + unsigned int pos; /* We need to avoid here duplicates. */ - is_dup = 0; + is_dup = false; pos = find_import_in_list (fdef->imports, fdef->num_imports, name, internal_name ? internal_name : name, module, ordinal, &is_dup); - if (is_dup != 0) + if (is_dup) return -1; - if (fdef->imports && pos != fdef->num_imports) + if (fdef->imports && pos != (unsigned)fdef->num_imports) { i = fdef->imports + pos; if (i->module && strcmp (i->module->name, module) == 0) return -1; } - if (fdef->num_imports + num_imports - 1 >= max_imports) + if ((unsigned)fdef->num_imports + num_imports - 1 >= fdef->max_imports) { - max_imports = ROUND_UP (fdef->num_imports + num_imports, 16); + fdef->max_imports = fdef->num_imports + num_imports + + SYMBOL_LIST_ARRAY_GROW; - if (fdef->imports) - fdef->imports = xrealloc (fdef->imports, - max_imports * sizeof (def_file_import)); - else - fdef->imports = xmalloc (max_imports * sizeof (def_file_import)); + fdef->imports = xrealloc (fdef->imports, + fdef->max_imports * sizeof (def_file_import)); } i = fdef->imports + pos; - if (pos != fdef->num_imports) + /* If we're inserting in the middle of the array, we need to move the + following elements forward. */ + if (pos != (unsigned)fdef->num_imports) memmove (i + num_imports, i, sizeof (def_file_import) * (fdef->num_imports - pos)); @@ -1261,7 +1256,7 @@ def_exports (const char *external_name, const char *its_name) { def_file_export *dfe; - int is_dup = 0; + bool is_dup = false; if (!internal_name && external_name) internal_name = external_name; @@ -1297,7 +1292,7 @@ def_import (const char *internal_name, { char *buf = 0; const char *ext = dllext ? dllext : "dll"; - int is_dup = 0; + bool is_dup = false; buf = xmalloc (strlen (module) + strlen (ext) + 2); sprintf (buf, "%s.%s", module, ext); diff --git a/ld/pe-dll.c b/ld/pe-dll.c index 60584a88571..92c33f528c8 100644 --- a/ld/pe-dll.c +++ b/ld/pe-dll.c @@ -791,7 +791,7 @@ process_def_file_and_drectve (bfd *abfd ATTRIBUTE_UNUSED, struct bfd_link_info * if (auto_export (b, pe_def_file, sn)) { - int is_dup = 0; + bool is_dup = false; def_file_export *p; p = def_file_add_export (pe_def_file, sn, 0, -1, @@ -857,7 +857,7 @@ process_def_file_and_drectve (bfd *abfd ATTRIBUTE_UNUSED, struct bfd_link_info * if (strchr (pe_def_file->exports[i].name, '@')) { - int is_dup = 1; + bool is_dup = true; int lead_at = (*pe_def_file->exports[i].name == '@'); char *tmp = xstrdup (pe_def_file->exports[i].name + lead_at); @@ -3579,7 +3579,7 @@ pe_implied_import_dll (const char *filename) exported in buggy auto-import releases. */ if (! startswith (erva + name_rva, "__nm_")) { - int is_dup = 0; + bool is_dup = false; /* is_data is true if the address is in the data, rdata or bss segment. */ is_data = -- 2.25.1