From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x236.google.com (mail-lj1-x236.google.com [IPv6:2a00:1450:4864:20::236]) by sourceware.org (Postfix) with ESMTPS id 055B5385E824 for ; Thu, 19 Mar 2020 09:12:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 055B5385E824 Received: by mail-lj1-x236.google.com with SMTP id z10so701772ljn.0 for ; Thu, 19 Mar 2020 02:12:23 -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=Y6BR2giKs8He6BPcIM0iV48XmnRJTH/nRIqRHDM0vy0=; b=H/8mKwtj4Zj1+14uMsedloWDV69ca+yHOYDIbjV+hoUy0Dh78i2qktEPDjScdEdx3h OyVmf1WzZBKoGxDnuob00ki9+8FlKp40FcF5AgnVImvyLV7GrV5zDBsaYHkfDSvSSTm5 i6usRm6K+w1XtySy0xyESn5IF94czKjWbLoBcQ1tqNF2gDlwWez/Si/kQ0x+q7Ms51zU buctcFNOjWrmpAln+1fwe1k9Qzz2PicFjqs9wPIrj7nb12nbZ+Yx5i/6o8QZYea+RM99 BbBMfGL+MaoUh9DX9F7WnK9LoPIOpifFPi1uFz8tvCIxMDSEaGi0cPUWM/Rn0ll+wcbx o2Ug== X-Gm-Message-State: ANhLgQ02mMHbmS3dEdq0a8+3s9V3SoCLbrRzkZ6HF0AQKHiHZD8pkRSW Ec9MheGW4oLO/RPYMUF2boYjV5iqGU9JNftlDSw= X-Google-Smtp-Source: ADFU+vvZt0DXfv2uR+uEcEtiWimv/WFHcOsIBm/W2BqnOb/rD4Ve9SUGY+qF6Cb4gqUoC+FoVZ38IJUNM3L0A+cy/Mg= X-Received: by 2002:a2e:a368:: with SMTP id i8mr1243555ljn.10.1584609142662; Thu, 19 Mar 2020 02:12:22 -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> In-Reply-To: From: Richard Biener Date: Thu, 19 Mar 2020 10:12:09 +0100 Message-ID: Subject: Re: [PATCH][RFC] API extension for binutils (type of symbols). To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: 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=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Mar 2020 09:12:25 -0000 On Wed, Mar 18, 2020 at 9:52 AM Martin Li=C5=A1ka wrote: > > 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_symta= b_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 least > > bit reasonably ordered. > > Ok, I'll adjust it and I will send a separate patch where we bump LTO_maj= or_version. > > >> > >> +/* Write extension information for symbols (symbol type, section flag= s). */ > >> + > >> +static void > >> +write_symbol_extension_info (tree t) > >> +{ > >> + unsigned char c; > > Do we still use vertical whitespace after decls per GNU coding style? > > 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 have m= ore 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 is po= inted > >> + by P and the result is written in ENTRY. The slot number is stored= in SLOT. > >> + Returns the address of the next entry. */ > >> + > >> +static char * > >> +parse_table_entry_extension (char *p, struct ld_plugin_symbol *entry) > >> +{ > >> + 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 without > > introducing too many additional sections. > > > > Currently there are 2 bytes per entry, while only 3 bits are actively > > used of them. If we invent next flag to pass we can use unused bits > > 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 bits 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 LTO > > 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 and > 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? + 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_symbol *en= try, 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++; ? +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...). Otherwise looks OK to me. Thanks, Richard. > Martin > > > Honza > >> +#ifdef __BIG_ENDIAN__ > >> + char unused; > >> + char section_flags; > >> + char symbol_type; > >> + char def; > >> +#else > >> + char def; > >> + char symbol_type; > >> + char section_flags; > >> + char unused; > >> +#endif > >> int visibility; > >> uint64_t size; > >> char *comdat_key; > >> @@ -123,6 +134,20 @@ enum ld_plugin_symbol_visibility > >> LDPV_HIDDEN > >> }; > >> > >> +/* The type of the symbol. */ > >> + > >> +enum ld_plugin_symbol_type > >> +{ > >> + LDST_UNKNOWN, > >> + LDST_FUNCTION, > >> + LDST_VARIABLE, > >> +}; > >> + > >> +enum ld_plugin_symbol_section_flags > >> +{ > >> + LDSSS_BSS =3D 1 > >> +}; > >> + > >> /* How a symbol is resolved. */ > >> > >> enum ld_plugin_symbol_resolution > >> @@ -431,7 +456,9 @@ enum ld_plugin_tag > >> LDPT_GET_INPUT_SECTION_ALIGNMENT =3D 29, > >> LDPT_GET_INPUT_SECTION_SIZE =3D 30, > >> LDPT_REGISTER_NEW_INPUT_HOOK =3D 31, > >> - LDPT_GET_WRAP_SYMBOLS =3D 32 > >> + LDPT_GET_WRAP_SYMBOLS =3D 32, > >> + LDPT_ADD_SYMBOLS_V2 =3D 33, > >> + LDPT_GET_SYMBOLS_V4 =3D 34, > >> }; > >> > >> /* The plugin transfer vector. */ > >> -- > >> 2.25.1 > >> > > >