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 E49F43836E73 for ; Tue, 24 May 2022 16:23:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E49F43836E73 Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-179-Vy6y84mvPyqXMkrWZHSkgg-1; Tue, 24 May 2022 12:23:42 -0400 X-MC-Unique: Vy6y84mvPyqXMkrWZHSkgg-1 Received: by mail-wm1-f72.google.com with SMTP id n25-20020a05600c3b9900b0039733081b4dso1480783wms.7 for ; Tue, 24 May 2022 09:23:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent :content-language:to:references:from:subject:in-reply-to :content-transfer-encoding; bh=M+iXZemcT2ocKiunaM7SgLIM6jDrNWrquQG9715l0YU=; b=VREEiJ6auaUIaYDV4U/O53xc8jLJ8auxYvCGUUAzUbiIyptBbo+lmf7nEO+48Vvk4o hHGhyhyWGTxs3dLWQce8XNbxX+ofvTHE1Rxv/BC8/RKIvMGFXqmC+SpPR4/OymYUpu1v pKiyEInClBxAS2w6ML+pGTGS26hel0M/5X6d7l/H0u9u34fEIi74k7L7EBF5k250VyMd slD2U4ul84TxOnbwuVwKRQkpJhJsztCLq9mEsib3P3p4PmP94623mGTXmMGCOka3VlJh emZxg1ApxHGl6193B4PZUKx+s/E+0WpAPo3c4HoIlYoGUkooqCTR8J6USoq/EBAAKNJf wsIg== X-Gm-Message-State: AOAM5316sHngJ70hH0+jVrOwhyyVeMy7YD2YK2qa+W5M5oxzZs8Q4EwP s6ZE2wyyWUyHOjP41KHrT8KRaQVea5BI9YMnNKMmtYHa0rab74lehVUoEiqR7xT4LbAMJJGnqTP h0g2ze9EJBe1MsBf2ug== X-Received: by 2002:adf:e849:0:b0:20d:129c:6a2d with SMTP id d9-20020adfe849000000b0020d129c6a2dmr23188039wrn.533.1653409421073; Tue, 24 May 2022 09:23:41 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwvMsDGwHneDxvh1iKAcLmrYhx/w94usl99NQu5ZZv/SQ6FXpmLpnibDPcu+m8g4TXGlpLlUg== X-Received: by 2002:adf:e849:0:b0:20d:129c:6a2d with SMTP id d9-20020adfe849000000b0020d129c6a2dmr23188019wrn.533.1653409420760; Tue, 24 May 2022 09:23:40 -0700 (PDT) Received: from [192.168.1.6] (adsl-2-solo-173-39.claranet.co.uk. [80.168.173.39]) by smtp.gmail.com with ESMTPSA id q14-20020adfab0e000000b0020d0c9c95d3sm13415173wrc.77.2022.05.24.09.23.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 May 2022 09:23:40 -0700 (PDT) Message-ID: <7cd459a6-ac46-e9c2-14d1-16d78118bb92@redhat.com> Date: Tue, 24 May 2022 17:23:36 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 To: luca.boccassi@gmail.com, binutils@sourceware.org References: <20220515191846.114257-1-luca.boccassi@gmail.com> From: Nick Clifton Subject: Re: [PATCH] ld: add --package-metadata In-Reply-To: <20220515191846.114257-1-luca.boccassi@gmail.com> 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=-5.8 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 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: Tue, 24 May 2022 16:23:45 -0000 Hi Luca, First of all a couple of questions about the need for the patch itself. Firstly - why modify the linker itself ? Couldn't the same effect be achieved by using the assembler to create an object file and then including that in the link along with the other objects ? Secondly - why build json verification into the linker ? It does not feel like a linkery kind of thing to do, and because the verification is dependent upon the linker being configured and built with the feature enabled, it means that programmers cannot rely upon their packaging notes always being checked. To me, I think that it would be better to have a pre- or post- links stage where the note is checked by a separate tool. Lastly, since this option is being used to create a note section, maybe we ought to consider the need for a generic mechanism for the linker to create sections - other than using linker scripts. For example, suppose that instead of --package-metadata= there was an option: --add-note=,,,[] which would basically do the same thing, but in a more generic fashion. Some thoughts on the patch itself: > + if (t->o->build_id.after_write_object_contents != NULL && !(*t->o->build_id.after_write_object_contents) (abfd)) > + return false; Line length - the if-statement should be over two lines. > + if (t->o->package_metadata.after_write_object_contents != NULL) > + return (*t->o->package_metadata.after_write_object_contents) (abfd); For consistency sake, it would be nice if these two lines were turned into a two predicate if-statement like the ones above. That way if other after_write_xxx functions are added in the future, then this if-statement will not need to be modified. > +@kindex --package-metadata=@var{JSON} > +@item --package-metadata=@var{JSON} > +Request the creation of a @code{.note.package} ELF note section. The > +contents of the note are in JSON format, as per the package metadata > +specification. For more information see: > +https://systemd.io/COREDUMP_PACKAGE_METADATA/ You should mention that if the JSON argument is missing/empty then this will disable the creation of the metadata note, if one had been enabled by an earlier occurrence of the --package-metdata option. You should also add an entry to the ld/NEWS file about this new feature. It might also be worth mentioning that the contents of the JSON text will be checked for conformance to the standard, if the linker has been configured and built that way. Also - can the JSON data be read from a file ? I can easily see problems being encountered with the length of the linker command line if the JSON data can only be specified as text. (Possibly this can be avoided by using the @file syntax already supported by the linker). > - if (ldelf_emit_note_gnu_build_id != NULL) > + if (ldelf_emit_note_gnu_build_id != NULL || > + ldelf_emit_note_fdo_package_metadata != NULL) Formatting - the || should be at the start of the next line. > + if (!ldelf_emit_note_fdo_package_metadata || strlen(ldelf_emit_note_fdo_package_metadata) == 0) > + return false; Formatting - a space between strlen and the opening parenthesis please. > + json_t *json = json_loads(ldelf_emit_note_fdo_package_metadata, 0, &json_error); > + json_decref(json); Likewise for these two function calls. > +if { [istarget frv-*-*] || [istarget lm32-*-*] } { > + return > +} Why are these architectures being skipped ? Cheers Nick