Re: [PATCH 0/8] Improve build process

["Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On 23/02/2015 18:23, Neil Horman wrote:
> On Mon, Feb 23, 2015 at 02:58:30PM +0000, Gonzalez Monroy, Sergio wrote:
>> On 23/02/2015 13:52, Neil Horman wrote:
>>> On Mon, Feb 23, 2015 at 10:25:01AM +0000, Gonzalez Monroy, Sergio wrote:
>>>> On 22/02/2015 23:37, Neil Horman wrote:
>>>>> On Fri, Feb 20, 2015 at 02:31:36PM +0000, Gonzalez Monroy, Sergio wrote:
>>>>>> On 13/02/2015 12:51, Neil Horman wrote:
>>>>>>> On Fri, Feb 13, 2015 at 11:08:02AM +0000, Gonzalez Monroy, Sergio wrote:
>>>>>>>> On 13/02/2015 10:14, Panu Matilainen wrote:
>>>>>>>>> On 02/12/2015 05:52 PM, Neil Horman wrote:
>>>>>>>>>> On Thu, Feb 12, 2015 at 04:07:50PM +0200, Panu Matilainen wrote:
>>>>>>>>>>> On 02/12/2015 02:23 PM, Neil Horman wrote:
>>>>>>>>> [...snip...]
>>>>>>>>>>>>>>> So I just realized that I was not having into account a possible
>>>>>>>>>>>>>>> scenario, where
>>>>>>>>>>>>>>> we have an app built with static dpdk libs then loading a dso
>>>>>>>>>>>>>>> with -d
>>>>>>>>>>>>>>> option.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In such case, because the pmd would have DT_NEEDED entries,
>>>>>>>>>>>>>>> dlopen will
>>>>>>>>>>>>>>> fail.
>>>>>>>>>>>>>>> So to enable such scenario we would need to build PMDs without
>>>>>>>>>>>>>>> DT_NEEDED
>>>>>>>>>>>>>>> entries.
>>>>>>>>>>>>>> Hmm, for that to be a problem you'd need to have the PMD built
>>>>>>>>>>>>>> against
>>>>>>>>>>>>>> shared dpdk libs and while the application is built against
>>>>>>>>>>>>>> static dpdk
>>>>>>>>>>>>>> libs. I dont think that's a supportable scenario in any case.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Or is there some other scenario that I'm not seeing?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>     - Panu -
>>>>>>>>>>>>>>
>>>>>>>>>>>>> I agree with you. I suppose it comes down to, do we want to
>>>>>>>>>>>>> support such
>>>>>>>>>>>>> scenario?
>>>>>>>>>>>>>
>>>>>>>>>>>>>  From what I can see, it seems that we do currently support such
>>>>>>>>>>>>> scenario by
>>>>>>>>>>>>> building dpdk apps against all static dpdk libs using
>>>>>>>>>>>>> --whole-archive (all
>>>>>>>>>>>>> libs and not only PMDs).
>>>>>>>>>>>>> http://dpdk.org/browse/dpdk/commit/?id=20afd76a504155e947c770783ef5023e87136ad8
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Am I misunderstanding this?
>>>>>>>>>>>>>
>>>>>>>>>>>> Shoot, you're right, I missed the static build aspect to this.  Yes,
>>>>>>>>>>>> if we do the following:
>>>>>>>>>>>>
>>>>>>>>>>>> 1) Build the DPDK as a static library
>>>>>>>>>>>> 2) Link an application against (1)
>>>>>>>>>>>> 3) Use the dlopen mechanism to load a PMD built as a DSO
>>>>>>>>>>>>
>>>>>>>>>>>> Then the DT_NEEDED entries in the DSO will go unsatisfied, because
>>>>>>>>>>>> the shared
>>>>>>>>>>>> objects on which it (the PMD) depends will not exist in the file
>>>>>>>>>>>> system.
>>>>>>>>>>> I think its even more twisty:
>>>>>>>>>>>
>>>>>>>>>>> 1) Build the DPDK as a static library
>>>>>>>>>>> 2) Link an application against (1)
>>>>>>>>>>> 3) Do another build of DPDK as a shared library
>>>>>>>>>>> 4) In app 2), use the dlopen mechanism to load a PMD built as a part
>>>>>>>>>>> of or
>>>>>>>>>>> against 3)
>>>>>>>>>>>
>>>>>>>>>>> Somehow I doubt this would work very well.
>>>>>>>>>>>
>>>>>>>>>> Ideally it should, presuming the ABI is preserved between (1) and (3),
>>>>>>>>>> though I
>>>>>>>>>> agree, up until recently, that was an assumption that was unreliable.
>>>>>>>>> Versioning is a big and important step towards reliability but there are
>>>>>>>>> more issues to solve. This of course getting pretty far from the original
>>>>>>>>> topic, but at least one such issue is that there are some cases where a
>>>>>>>>> config value affects what are apparently public structs (rte_mbuf wrt
>>>>>>>>> RTE_MBUF_REFCNT for example), which really is a no-go.
>>>>>>>>>
>>>>>>>> Agree, the RTE_MBUF_REFCNT is something that needs to be dealt with asap.
>>>>>>>> I'll look into it.
>>>>>>>>
>>>>>>>>>>>> I think the problem is a little bit orthogonal to the libdpdk_core
>>>>>>>>>>>> problem you
>>>>>>>>>>>> were initially addressing.  That is to say, this problem of
>>>>>>>>>>>> dlopen-ed PMD's
>>>>>>>>>>>> exists regardless of weather you build the DPDK as part of a static
>>>>>>>>>>>> or dynamic
>>>>>>>>>>>> library.  The problems just happen to intersect in their
>>>>>>>>>>>> manipulation of the
>>>>>>>>>>>> DT_NEEDED entries.
>>>>>>>>>>>>
>>>>>>>>>>>> Ok, so, given the above, I would say your approach is likely
>>>>>>>>>>>> correct, just
>>>>>>>>>>>> prevent DT_NEEDED entries from getting added to PMD's. Doing so will
>>>>>>>>>>>> sidestep
>>>>>>>>>>>> loading issue for libraries that may not exist in the filesystem,
>>>>>>>>>>>> but thats ok,
>>>>>>>>>>>> because by all rights, the symbols codified in those needed
>>>>>>>>>>>> libraries should
>>>>>>>>>>>> already be present in the running application (either made available
>>>>>>>>>>>> by the
>>>>>>>>>>>> application having statically linked them, or having the linker load
>>>>>>>>>>>> them from
>>>>>>>>>>>> the proper libraries at run time).
>>>>>>>>>>> My 5c is that I'd much rather see the common case (all static or all
>>>>>>>>>>> shared)
>>>>>>>>>>> be simple and reliable, which in case of DSOs includes no lying
>>>>>>>>>>> (whether by
>>>>>>>>>>> omission or otherwise) about DT_NEEDED, ever. That way the issue is
>>>>>>>>>>> dealt
>>>>>>>>>>> once where it belongs. If somebody wants to go down the rabbit hole of
>>>>>>>>>>> mixed
>>>>>>>>>>> shared + static linkage, let them dig the hole by themselves :)
>>>>>>>>>>>
>>>>>>>>>> This is a fair point.  Can DT_NEEDED sections be stripped via tools like
>>>>>>>>>> objcopy
>>>>>>>>>> after the build is complete?  If so, end users can hack this corner case
>>>>>>>>>> to work
>>>>>>>>>> as needed.
>>>>>>>>> Patchelf (http://nixos.org/patchelf.html) appears to support that, but
>>>>>>>>> given that source is available it'd be easier to just modify the makefiles
>>>>>>>>> if that's really needed.
>>>>>>>>>
>>>>>>>> I think we agree on the issue.
>>>>>>>>
>>>>>>>> So I'll be sending a patch to add DT_NEEDED entries to all libraries and
>>>>>>>> PMDs. The only exception would be librte_eal, which would not have proper
>>>>>>>> NEEDED entries.
>>>>>>>> Do we bother adding a linker script for librte_eal that would include
>>>>>>>> dependent libraries?
>>>>>>>>
>>>>>>> I say yes to the linker script, but will happily bow to an alternate consensus
>>>>>>> Neil
>>>>>>>
>>>>>> So the case we want to solve is the following circular dependencies:
>>>>>> eal             -> mempool, malloc
>>>>>> mempool -> eal , malloc, ring
>>>>>> malloc      -> eal
>>>>>> ring           -> eal, malloc
>>>>>>
>>>>>> We cannot write/create the proposed (below) linker script at least until we
>>>>>> have built mempool and malloc.
>>>>>> INPUT ( -lrte_eal.so -lrte_mempool -lrte_malloc )
>>>>>>
>>>>> Not sure I understand why you have a build time dependency on this.  Link time
>>>>> perhaps, but not build time.  Or am I reading too much into your use of the term
>>>>> 'built' above?
>>>> I meant 'built' as compiled + linked. Am I misusing the term?
>>> No, you're not (though I misused the term link time above, I meant to say load
>>> time).  So you're saying that when you build shared libraries, you get linker
>>> errors indicating that, during the build, you're missing symbols, is that
>>> correct?  I guess I'm confused because I don't see how thats not happening for
>>> everyone, right now.  In other words, I'm not sure what about your changes is
>>> giving rise to that problem.
>>>
>>>>>> Few ways I have thought about implementing this (not particularly fond of
>>>>>> any of them) :
>>>>>>   - Have the linker script file in the repo (scripts/ ?) in a fixed location
>>>>>> and just copy it to $(RTE_OUTPUT)/lib/ once all libs have finished building.
>>>>>>   - Generate the file on build time from a defined make variable once all
>>>>>> libs have finished
>>>>>>
>>>>> I'm still not sure I understand.  Why does this dependency exist at build time?
>>>>> The dependency between malloc and eal shouldn't be a problem during the build,
>>>>> as symbols from each other should just remain undefined, and get resolved at
>>>>> load time.
>>>> Is that not the way it is currently implemented?
>>>> I get the impression that we are talking about different goals (correct me
>>>> if it is not the case)
>>>>
>>> We may well be, I'm not sure yet.
>>>
>>>> I thought that the agreed solution was to:
>>>> 1) NOT to create/generate a 'core' library
>>>> 2) Add DT_NEEDED entries for all libraries (except eal which is the first
>>>> library we link)
>>>> 3) Use linker script for eal
>>>>
>>> Ok, we're definately on the same page, as thats what I thought the goal was as
>>> well.
>>>
>>>> Given the previously mentioned circular dependencies between eal, mempool,
>>>> malloc and ring:
>>>> - eal would not be linked against other libraries (no NEEDED entries)
>>>> - malloc is linked against eal (previously built), so malloc would have a
>>>> NEEDED entry for eal.
>>>>
>>>> In that scenario, if the linker script is setup/created after we build eal,
>>>> then when we try to link malloc
>>>> against eal, the linker will pull mempool and malloc too (because we
>>>> included them in the linker script).
>>>> Therefore, the link fails as none of those libraries (malloc and mempool)
>>>> have been built yet.
>>>>
>>> Ah, I see now, I wasn't thinking about the extra requirements that DT_NEEDED
>>> entries placed on the build conditions.
>>>
>>> I see now, apologies for being dense previously.  Given what you indicate I
>>> would say that the solution here is to link the libraries against individual
>>> other specific libraries, not the core library that you generate as a linker
>>> script.  That way you avoid the circular dependency, and the core library just
>>> becomes a convienience for application developers looking to link to a single
>>> library.
>>>
>> I'm not sure I quite understand your suggestion.
>>
>> Could you roughly describe steps for building eal, malloc and mempool libs ?
>> For example, something like this?
>> 1) build eal, which creates librte_eal.so.1
>> 2) write linker script for librte_eal.so
>> 3) build malloc against eal (-lrte_eal )
>> etc
> Hm, so I spent a bit of time looking at this, and your right, I thought this was
> really just a artifact of the introduction of --as-needed to the build to force
> DT_NEEDED entries, and my suggestion was that you simply not link the libraries
> that were causing the circular dependency.  I had assumed that the link
> directives included -lrte_malloc -lrte_mempool for the eal library, but they
> weren't really needed, so you could remove them and it would work out.
>
> Unfortunately that turns out to not be the case.  librte_eal does explicitly use
> calls in librte_malloc, and vice versa.  The current use of -no-as-needed in the
> build system (which I was previously unaware of), is a hack to avoid having to
> address that problem.
>
> That throws a monkey wrench into this plan.  I would see 3 ways forward:
>
> 1) Fix the problem - That is to say, remove the use of --no-as-needed from the
> build, and address the circular dependencies that arise.  This could/will mean
> actually merging libraries with circular dependencies into a single library, as
> they should be so that they can completely resolve all of the symbols they use
> at link time
>
> 2) Ignore the problem.  If we just keep the lack of DT_NEEDED entries in place,
> I think the problem goes away, and we can continue on.
>
> I think option 1 is likely the more correct approach, as removing DT_NEEDED to
> avoid a circuar depenency is a hack, but it may not be the most pragmatic
> approach.  just living without DT_NEEDED entries and documenting link time needs
> will certainly be faster, and might be the better course of action, especially
> if we provide a 'core' pseudo library/linker script that embodies that action
> for the end user.
>
> Neil
>
So basically for 1) the approach of creating a core library would be a 
solution.
The last suggestion for the core library was to not merge the sources 
but generate a single library.
The problem with that was the versioning wouldn't work as it currently 
is, given that is per library.
So if we were to create a core library, we just need to merge the 
version.map files of each library
into a single version.map for the core library. This approach, as you 
noted, would be the proper fix.

The other solution would be to just leave eal without DT_NEEDED entries 
and specify in the docs
that apps require eal, mempool, malloc and ring to be link such as:
--no-as-needed -lrte_eal -lrte_mempool -lrte_malloc -lrte_ring --as-needed
With those flags it should work regardless of --as-needed being set 
before hand.

With this second approach we would have all libraries, but eal, with 
DT_NEEDED entries and we
would not need to create a core library. We don't really need to create 
a linker script for that
(not sure we can even create such linker script with those flags) and 
just documenting link time
needs as you mentioned should be enough.

So should I go forward with last suggested approach?

Regards,
Sergio
>> I suppose that another way to go about this, instead of creating the linker
>> script that pulls
>> dependent libraries, is to always link (using --no-as-needed in case gcc
>> adds it by default)
>> against these libraries (eal, mempool, malloc, and ring) with necessary doc
>> about how to build apps.
>>
>> Sergio
>>> Neil
>>>
>>>> Was your suggestion to leave all of these libraries (eal, mempool, malloc,
>>>> ring) without NEEDED entries?
>>>>
>>> No, you can add NEEDED entries there, they will just be for the individual
>>> libraries, not the core linker script library.
>>>
>>> Best
>>> Neil
>>>
>>>> Regards,
>>>> Sergio
>>>>> What is the error you are getting?
>>>>>
>>>>> Best
>>>>> Neil
>>>>>
>>>>>> Thoughts? any other approached is more than welcome!
>>>>>>
>>>>>> Sergio
>>>>>>
>>>>>> PS: Thinking again on the core library and the issue of having multiple
>>>>>> version.map files, we could have a core_version.map instead instead of
>>>>>> multiple files per core library (eal, mempool, etc)
>>>>>>
>>>>>>
>>>>
>>
>>