From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x343.google.com (mail-ot1-x343.google.com [IPv6:2607:f8b0:4864:20::343]) by sourceware.org (Postfix) with ESMTPS id 0195E393FC25 for ; Thu, 19 Mar 2020 15:51:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0195E393FC25 Received: by mail-ot1-x343.google.com with SMTP id r2so2852811otn.6 for ; Thu, 19 Mar 2020 08:51:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=nRyOUL8a388PnIt0+K68HfRBHQ+ibdjmep/RZ2bWY3M=; b=b4JVG2VvbQWRIkV5E8YA6UA/yw6445knrtdGoBHomTLPq+EY0N/xDLUgu5YQ88F2Un TmAuHQdkYwlu1qrR5U4kIcPcr2qEo1CAzdX7vIEkwU0GIdg0sodzUdvmDftySCmHETB9 aRYlS3lZxN1hGH2FjfC8GZY4oJP13pfJOPskbSIagL/nOPOxkqRhM6zx87uRu0ZMxZRE b8Dw9hkylZI69D8l9hmBwi574oVBt8qUaYXnucbxLv58Hv/RK96MQ/3BL0BwK/659/Ct zEeZyvAi4mbpKkS50tIrPRNNPyEk5hLo1aaTDE8BpsHBCQvxJ8Y2RFj5FdZn+09+fMJw 6A/A== X-Gm-Message-State: ANhLgQ0OCBHbkJQhuP4bSQyDN5GFkMsfN+6wXHtAhvu9m70z/P4g3c9r XL09Z64Z4at6LkROag7aNUaGJgOeOABVxEcnHxE= X-Google-Smtp-Source: ADFU+vu/dJ92AmdStj9UO8nJQFnuqbA+JEjfTWjO9Ik7+3NS4NThoyGlJGqpZQ8wKV64WPzycuwq9lpDlllgvXyEwzs= X-Received: by 2002:a9d:23e4:: with SMTP id t91mr2827557otb.125.1584633074908; Thu, 19 Mar 2020 08:51:14 -0700 (PDT) MIME-Version: 1.0 References: <20200310110929.GA48643@kam.mff.cuni.cz> <36d32a03-a4b5-1318-38b6-ece55fe7ed70@suse.cz> <78b445d1-ab4f-ad21-e3a1-aa791a15361c@suse.cz> <15eb771d-6d3a-b9e3-3349-cabfb123cdc6@suse.cz> <46cd4dbe-40ce-46c3-33fd-7d10f362262f@suse.cz> <436149e2-6df2-6a5a-7612-e85661ed9bc3@suse.cz> <20200317232705.GP66618@kam.mff.cuni.cz> <6b621ea7-64a7-4792-022f-0449ab6da4a4@suse.cz> In-Reply-To: From: "H.J. Lu" Date: Thu, 19 Mar 2020 08:50:38 -0700 Message-ID: Subject: Re: [PATCH][RFC] API extension for binutils (type of symbols). To: Richard Biener Cc: =?UTF-8?Q?Martin_Li=C5=A1ka?= , Binutils , Jan Hubicka , GCC Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-25.2 required=5.0 tests=DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Mar 2020 15:51:17 -0000 On Thu, Mar 19, 2020 at 8:46 AM Richard Biener wrote: > > On Thu, Mar 19, 2020 at 4:00 PM Martin Li=C5=A1ka wrote: > > > > On 3/19/20 10:12 AM, Richard Biener wrote: > > > On Wed, Mar 18, 2020 at 9:52 AM Martin Li=C5=A1ka wr= ote: > > >> > > >> On 3/18/20 12:27 AM, Jan Hubicka wrote: > > >>>> Hi. > > >>>> > > >>>> There's updated version of the patch. > > >>>> Changes from the previous version: > > >>>> - comment added to ld_plugin_symbol > > >>>> - new section renamed to ext_symtab > > >>>> - assert added for loop iterations in produce_symtab and produce_s= ymtab_extension > > >>> Hi, > > >>> I hope this is last version of the patch. > > >> > > >> Hello. > > >> > > >> Yes. > > >> > > >>>> > > >>>> 2020-03-12 Martin Liska > > >>>> > > >>>> * lto-section-in.c: Add extension_symtab. > > >>> ext_symtab :) > > >> > > >> Fixed. > > >> > > >>>> diff --git a/gcc/lto-section-in.c b/gcc/lto-section-in.c > > >>>> index c17dd69dbdd..78b015be696 100644 > > >>>> --- a/gcc/lto-section-in.c > > >>>> +++ b/gcc/lto-section-in.c > > >>>> @@ -54,7 +54,8 @@ const char *lto_section_name[LTO_N_SECTION_TYPES= ] =3D > > >>>> "mode_table", > > >>>> "hsa", > > >>>> "lto", > > >>>> - "ipa_sra" > > >>>> + "ipa_sra", > > >>>> + "ext_symtab" > > >>> I would move ext_symtab next to symtab so the sections remains at l= east > > >>> bit reasonably ordered. > > >> > > >> Ok, I'll adjust it and I will send a separate patch where we bump LT= O_major_version. > > >> > > >>>> > > >>>> +/* Write extension information for symbols (symbol type, section = flags). */ > > >>>> + > > >>>> +static void > > >>>> +write_symbol_extension_info (tree t) > > >>>> +{ > > >>>> + unsigned char c; > > >>> Do we still use vertical whitespace after decls per GNU coding styl= e? > > >> > > >> Dunno. This seems to me like a nit. > > >> > > >>>> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h > > >>>> index 25bf6c468f7..4f82b439360 100644 > > >>>> --- a/gcc/lto-streamer.h > > >>>> +++ b/gcc/lto-streamer.h > > >>>> @@ -236,6 +236,7 @@ enum lto_section_type > > >>>> LTO_section_ipa_hsa, > > >>>> LTO_section_lto, > > >>>> LTO_section_ipa_sra, > > >>>> + LTO_section_symtab_extension, > > >>> I guess symtab_ext to match the actual section name? > > >> > > >> No. See e.g. LTO_section_jump_functions - "jmpfuncs". We want to h= ave more descriptive > > >> enum names. > > >> > > >>>> LTO_N_SECTION_TYPES /* Must be last. */ > > >>>> }; > > >>>> > > >>>> diff --git a/include/lto-symtab.h b/include/lto-symtab.h > > >>>> index 0ce0de10121..47f0ff27df8 100644 > > >>>> --- a/include/lto-symtab.h > > >>>> +++ b/include/lto-symtab.h > > >>>> @@ -38,4 +38,16 @@ enum gcc_plugin_symbol_visibility > > >>>> GCCPV_HIDDEN > > >>>> }; > > >>>> > > >>>> +enum gcc_plugin_symbol_type > > >>>> +{ > > >>>> + GCCST_UNKNOWN, > > >>>> + GCCST_FUNCTION, > > >>>> + GCCST_VARIABLE, > > >>>> +}; > > >>>> + > > >>>> +enum gcc_plugin_symbol_section_flags > > >>>> +{ > > >>>> + GCCSSS_BSS =3D 1 > > >>>> +}; > > >>> > > >>> Probably comments here? > > >> > > >> No. There are just shadow copy of enum types from plugin-api.h which > > >> are documented. > > >> > > >>>> + > > >>>> #endif /* GCC_LTO_SYMTAB_H */ > > >>>> +/* Parse an entry of the IL symbol table. The data to be parsed i= s pointed > > >>>> + by P and the result is written in ENTRY. The slot number is st= ored in SLOT. > > >>>> + Returns the address of the next entry. */ > > >>>> + > > >>>> +static char * > > >>>> +parse_table_entry_extension (char *p, struct ld_plugin_symbol *en= try) > > >>>> +{ > > >>>> + unsigned char t; > > >>>> + enum ld_plugin_symbol_type symbol_types[] =3D > > >>>> + { > > >>>> + LDST_UNKNOWN, > > >>>> + LDST_FUNCTION, > > >>>> + LDST_VARIABLE, > > >>>> + }; > > >>>> + > > >>>> + t =3D *p; > > >>>> + check (t <=3D 3, LDPL_FATAL, "invalid symbol type found"); > > >>>> + entry->symbol_type =3D symbol_types[t]; > > >>>> + p++; > > >>>> + entry->section_flags =3D *p; > > >>>> + p++; > > >>>> + > > >>>> + return p; > > >>>> +} > > >>> > > >>> I think we have chance to make some plan for future extensions with= out > > >>> introducing too many additional sections. > > >>> > > >>> Currently there are 2 bytes per entry, while only 3 bits are active= ly > > >>> used of them. If we invent next flag to pass we can use unused bit= s > > >>> however we need a way to indicate to plugin that the bit is defined= . > > >>> This could be done by a simple version byte at the beggining of > > >>> ext_symtab section which will be 0 now and once we define extra bit= s we > > >>> bump it up to 1. > > >> > > >> I like the suggested change, it can help us in the future. > > >> > > >>> > > >>> It is not that important given that even empty file results in 2k L= TO > > >>> object file, but I think it would be nicer in longer run. > > >>>> + /* This is for compatibility with older ABIs. */ > > >>> Perhaps say here that this ABI defined only "int def;" > > >> > > >> Good point. > > >> > > >>> > > >>> The patch look good to me. Thanks for the work! > > >> > > >> Thanks. I'm sending updated patch that I've just tested on lto.exp a= nd > > >> both binutils master and HJ's branch that utilizes the new API. > > > > > > @@ -495,10 +560,16 @@ write_resolution (void) > > > > > > /* Version 2 of API supports IRONLY_EXP resolution that is > > > accepted by GCC-4.7 and newer. */ > > > - if (get_symbols_v2) > > > - get_symbols_v2 (info->handle, symtab->nsyms, syms); > > > + if (get_symbols_v4) > > > + get_symbols_v4 (info->handle, symtab->nsyms, syms); > > > else > > > - get_symbols (info->handle, symtab->nsyms, syms); > > > + { > > > + clear_new_symtab_flags (symtab); > > > > > > can you instead just avoid parsing the ext symtab? > > > > Yes, I simplified the changes and I bet we'll only need one new hook ge= t_symbols_v2. > > Then we can base parsing of the external symtab on that. > > > > > > > > + if (get_symbols_v2) > > > + get_symbols_v2 (info->handle, symtab->nsyms, syms); > > > + else > > > + get_symbols (info->handle, symtab->nsyms, syms); > > > + } > > > > > > I guess this also points to the fact that LDs symbol resolution > > > can't tell GCC it chose "BSS" (from a non-IL object) or that > > > it chose a variable or function. > > > > > > @@ -296,6 +300,8 @@ parse_table_entry (char *p, struct ld_plugin_symb= ol *entry, > > > entry->visibility =3D translate_visibility[t]; > > > p++; > > > > > > + entry->unused =3D 0; > > > + > > > memcpy (&entry->size, p, sizeof (uint64_t)); > > > p +=3D 8; > > > > > > isn't that either not enough or too much clearing? > > > I'd have expected > > > > > > entry->unused =3D entry->section_flags =3D entry->symbol_type =3D 0= ; > > > > > > _before_ > > > > > > t =3D *p; > > > check (t <=3D 4, LDPL_FATAL, "invalid symbol kind found"); > > > entry->def =3D translate_kind[t]; > > > p++; > > > > > > ? > > > > Yes, that's better. > > > > > > > > +enum ld_plugin_symbol_section_flags > > > +{ > > > + LDSSS_BSS =3D 1 > > > +}; > > > + > > > > > > please add a symbolic name for the value zero, > > > may I suggest LDSSS_DEFAULT? I see you've > > > settled on symbol_section_flags rather than > > > section_kind (BSS is not really a flag...). > > > > I renamed that to ld_plugin_symbol_section_kind. > > > > May I install the 2 patches now? It's again tested on lto.exp > > and both old binutils and new HJ's binutils patch and it works as expec= ted. > > OK if HJ is happy, lto-symtab.h is owned by binutils I think and existing > users might need to be adjusted to clear the unused fields now that the > width of 'def' changed. > > Richard. I like it and I will take case of binutils side. Thanks. --=20 H.J.