On Tue, 2022-05-24 at 17:23 +0100, Nick Clifton wrote:
> 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 ?

So we are already using an intermediary artifact, and it worked well to
quickly get a workable PoC together, and then hammer it in shape to
deploy it distro-wide in Fedora and CBL-Mariner. Now that we are in
production phase, and we'de like for the spec to expand (and take over
the world!), we want to address an important shortcoming that affects
distro builders.

Dealing with intermediate artifacts means dealing with paths. And in
practice, that gets really, really, really awkward and problematic.
Almost all issues we've had are due to the fact that being able to
reliably reference a path at build time (while not breaking
reproducibility!) is _extremely_ complicated, and there's a huge tail
end of packages were things are just too hopelessly convoluted. It
might sound counter-intuitive as it's so trivial to put together an
hello world, but in reality when you are dealing with 20k to 30k
packages things get really, really weird. Coverage in F36 is not 100%
of binaries mostly because of this.

So with that experience in the bag, the most obvious next step is to
have a first-class option, that allows a self-contained flag to be
passed instead. After all the idea is similar to the build-id, and
there we have a first-class option too, and it works well.

>    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.

This is not aimed at individual programmers - an individual developer
is not expected to worry about any of this when hacking on local
software and such. The spec is made for distribution builders. And
speaking as one, my life would be so much easier if input was validated
before usage. Leaving it as an exercise for the reader means that
_every single distribution builder_ would need to reimplement the exact
same check, over and over and over again. Get it done once and everyone
can benefit. JSON looks the same across all distros, after all!

Or from another point of view: ld already validates other types of
input. Linker scripts get validated. Flags get validated. Objects get
validated. The type of build-id requested gets validated. So why not
allow (optionally!) to validate this too? It's easy, just half a dozen
lines of code, if the lib is missing nothing happens and it's just
skipped.

If there are any concerns I can flip the autoconf to be default-
disabled, so that --enable has to be passed explicitly, together with
making the dep available at build time?

>    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=<JSON> there was an option:
>    --add-note=<SEC-NAME>,<NOTE-NAME>,<CONTENTS>,[<VERIFIER>]
>    which would basically do the same thing, but in a more generic fashion.

Well we would lose verification, and also it sounds quite a bit more
complex (you'd need to pass IDs in hex format - quite a bit ugly) - if
there hasn't been a need so far, not sure it's worth it?

>    Some thoughts on the patch itself:

<..>

> 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).

It could be a separate option I guess, if needed, but we wouldn't use
it - as mentioned above, the main goal is to get rid of the
intermediate file entirely.

<..>

> 
> > +if { [istarget frv-*-*] || [istarget lm32-*-*] } {
> > +    return
> > +}
> 
> Why are these architectures being skipped ?

To be perfectly honest - cargo culting from another test :-) Removed
now.

Will send a v2 later with fixes for all the style issues and the ASAN
one.

Thanks for the review!


-- 
Kind regards,
Luca Boccassi