DT_NEEDED for shared builds in makefile

[haampie]

Description

The makefile build specifies -L. -lmbedx509 -lmbedcrypto flags first,
and only then object files referencing symbols from those libraries.

In this order the linker will not add the linked libraries to the
DT_NEEDED section because they are not referenced yet (at least that
happens for me on ubuntu 20.04 with the default gnu compiler tools).

By first specifying the object files and then the linked libraries, we
do end up with libmbedx509 and libmbedcrypto in the DT_NEEDED sections.

This way running dlopen(...) on libmedtls.so just works.

Note that the CMake build does this by default.

Also fixes a bug (?) where there is no whitespace between -lmbedx509$(LOCAL_LDFLAGS) (this was not the case)

Requires Backporting

It could be seen as a bug fix, since the cmake and makefile builds are inconsistent right now.

I'd be happy if this got backported.

Todos

• Tests (dlopen test #5131)
• Documentation
• Changelog updated
• Backported (Backport 2.x: DT_NEEDED for shared builds in makefile #5133)

Steps to test or reproduce

SHARED=1 make -j

[mpg]

Thanks for your contribution! After hesitating for a bit, I classified as "enhancement" rather that bug because the behaviour was as documented, but I agree it's on the line. Anyway, we can probably accept a backport to development_2.x unless reviewers have an objection. (But for 2.16 LTS I don't think we want to be making changes to the build system unless absolutely necessary.)

[haampie]

Thanks!

I just built mbedtls on a macbook (Apple clang version 11.0.0 on Catalina), and realized this PR is not required to make it work there:

$ git clone https://github.com/ARMmbed/mbedtls.git
$ cd mbedtls
$ SHARED=1 make -j no_test
$ otool -L library/libmbedtls.dylib 
library/libmbedtls.dylib:
	libmbedtls.dylib (compatibility version 0.0.0, current version 0.0.0)
	libmbedcrypto.dylib (compatibility version 0.0.0, current version 0.0.0)
	libmbedx509.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0)

so, it lists the required dylibs already.

That means that only on Linux w/ make, libmbedtls.so does not load its dependencies crypto & x509:

$ git clone https://github.com/ARMmbed/mbedtls.git
$ cd mbedtls
$ SHARED=1 make -j no_test
$ ldd library/libmbedtls.so
	linux-vdso.so.1 (0x00007fff999ba000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f2c50db9000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f2c50ff9000)

Just pointing this out as to me it looks more like a bug than an enhancement. But I can live with either :)

[gabor-mezei-arm]

Found in the linker's manual for the -l option:

The linker will search an archive only once, at the location where it is specified on the command line. If the archive defines a symbol which was undefined in some object which appeared before the archive on the command line, the linker will include the appropriate file(s) from the archive. However, an undefined symbol in an object appearing later on the command line will not cause the linker to search the archive again.

So the object files must be specified first and the shared libraries afterwards.

[bensze01]

The approach and resasoning of the PR seem valid to me, quoting the ld(1) manpage:

The linker will search an archive only once, at the location
where it is specified on the command line. If the archive
defines a symbol which was undefined in some object which
appeared before the archive on the command line, the linker
will include the appropriate file(s) from the archive.
However, an undefined symbol in an object appearing later on
the command line will not cause the linker to search the
archive again.

See the -( option for a way to force the linker to search
archives multiple times.

You may list the same archive multiple times on the command
line.

This type of archive searching is standard for Unix linkers.
However, if you are using ld on AIX, note that it is
different from the behaviour of the AIX linker.

However, the whitespace change made by this PR seems to be the opposite of what you described in the PR description, deleting the whitespace where it is actually needed.

As an aside, it looks like we are not following our own advice on the library order when linking the libraries - using the reverse of what is recommended. We should probably fix that as well, but I think that can be a separate PR.

[gilles-peskine-arm]

This makes sense to me. I don't know much about dynamic linking, but I do know that we should generally link libraries in reverse dependency order, which we weren't doing.

There are a couple of minor problems in the execution.

Also, I think this requires a changelog entry.

I want to have a no-regression test, so I made one: #5131. I verified manually that my test fails before your fix and passes after your fix. We can merge the test after the fix.

[gilles-peskine-arm]

I do think this is bug, and requires at least a 2.x backport and a changelog entry. I can live without a 2.16 backport since 2.16 is almost at end-of-life and anyone who was bothered by this bug has already worked around it.

[haampie]

Anything expected from my side on this? Should the changelog entry & dlopen-test be added to this PR?

[gilles-peskine-arm]

Looks good to me.

Please make a similar patch for the development_2.x branch.

[gilles-peskine-arm]

I'll handle the non-regression test in my PR.

However, please add a changelog entry here announcing the bug fix. This means a file in ChangeLog.d, see ChangeLog.d/00README.md and examples in ChangeLog.d/*.txt.

[haampie]

Added a changelog and a backport #5133

[haampie]

Thanks!