[libcamera-devel] src/libcamera/meson.build: link with atomic when needed

1952 diff mbox series

On some architectures, atomic binutils are provided by the libatomic
library from gcc. Linking with libatomic is therefore necessary,
otherwise the build fails with:

/home/buildroot/autobuild/run/instance-3/output/host/opt/ext-toolchain/bin/../lib/gcc/sparc-buildroot-linux-uclibc/7.4.0/../../../../sparc-buildroot-linux-uclibc/bin/ld: src/libcamera/4ab8042@@camera@sha/v4l2_videodevice.cpp.o: in function `libcamera::V4L2VideoDevice::queueBuffer(libcamera::Buffer*)':
v4l2_videodevice.cpp:(.text+0x1470): undefined reference to `__atomic_fetch_add_4'

This is often for example the case on sparc v8 32 bits.

Fixes:
 - http://autobuild.buildroot.org/results/1f0b8338f5f39aa86b9d432598dae2f53c5f7c84

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
---
 src/libcamera/meson.build | 1 +
 1 file changed, 1 insertion(+)

Hi Fabrice,

Thank you for this patch,

On 05/09/2019 20:04, Fabrice Fontaine wrote:
> On some architectures, atomic binutils are provided by the libatomic
> library from gcc. Linking with libatomic is therefore necessary,
> otherwise the build fails with:
> 
> /home/buildroot/autobuild/run/instance-3/output/host/opt/ext-toolchain/bin/../lib/gcc/sparc-buildroot-linux-uclibc/7.4.0/../../../../sparc-buildroot-linux-uclibc/bin/ld: src/libcamera/4ab8042@@camera@sha/v4l2_videodevice.cpp.o: in function `libcamera::V4L2VideoDevice::queueBuffer(libcamera::Buffer*)':
> v4l2_videodevice.cpp:(.text+0x1470): undefined reference to `__atomic_fetch_add_4'
> 
> This is often for example the case on sparc v8 32 bits.
> 
> Fixes:
>  - http://autobuild.buildroot.org/results/1f0b8338f5f39aa86b9d432598dae2f53c5f7c84
> 
> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> ---
>  src/libcamera/meson.build | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index c5d8f11..0706a08 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -99,6 +99,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()],
>  libcamera_sources += version_cpp
>  
>  libcamera_deps = [
> +    cc.find_library('atomic', required: false),

Interestingly, this adds the following clear message when I build on x86:

Library atomic found: YES

So - my system finds 'libatomic' and now explicitly adds it as the
dependencies. (I assumed it was a gcc builtin, for it not to be necessary)

I wondered if this caused any ill-effect, but actually I believe this is
perfectly safe. Thanks to using meson we have reproducible builds, and I
can see that both with and without this explicit linking we get
identical output binaries for libcamera.so

With that, and seeing the libcamera tests pass (which is not surprising
with no binary change - I've ensured that the git sha1 did not change
between builds) then I believe this is a good fix to support the broken
compile issues reported.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

I'll give the others a chance to provide any feedback if necessary and
then we can push this into the master branch, and then get buildroot
package building the latest version.


>      cc.find_library('dl'),
>      libudev,
>      dependency('threads'),
>

Hi Kieran,

On Fri, Sep 06, 2019 at 02:50:38PM +0100, Kieran Bingham wrote:
> On 05/09/2019 20:04, Fabrice Fontaine wrote:
> > On some architectures, atomic binutils are provided by the libatomic
> > library from gcc. Linking with libatomic is therefore necessary,
> > otherwise the build fails with:
> > 
> > /home/buildroot/autobuild/run/instance-3/output/host/opt/ext-toolchain/bin/../lib/gcc/sparc-buildroot-linux-uclibc/7.4.0/../../../../sparc-buildroot-linux-uclibc/bin/ld: src/libcamera/4ab8042@@camera@sha/v4l2_videodevice.cpp.o: in function `libcamera::V4L2VideoDevice::queueBuffer(libcamera::Buffer*)':
> > v4l2_videodevice.cpp:(.text+0x1470): undefined reference to `__atomic_fetch_add_4'
> > 
> > This is often for example the case on sparc v8 32 bits.
> > 
> > Fixes:
> >  - http://autobuild.buildroot.org/results/1f0b8338f5f39aa86b9d432598dae2f53c5f7c84
> > 
> > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> > ---
> >  src/libcamera/meson.build | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index c5d8f11..0706a08 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -99,6 +99,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()],
> >  libcamera_sources += version_cpp
> >  
> >  libcamera_deps = [
> > +    cc.find_library('atomic', required: false),
> 
> Interestingly, this adds the following clear message when I build on x86:
> 
> Library atomic found: YES
> 
> So - my system finds 'libatomic' and now explicitly adds it as the
> dependencies. (I assumed it was a gcc builtin, for it not to be necessary)
> 
> I wondered if this caused any ill-effect, but actually I believe this is
> perfectly safe. Thanks to using meson we have reproducible builds, and I
> can see that both with and without this explicit linking we get
> identical output binaries for libcamera.so

Is that because the linker doesn't find any symbol in libatomic.so that
are required by libcamera, and thus skips it ?

> With that, and seeing the libcamera tests pass (which is not surprising
> with no binary change - I've ensured that the git sha1 did not change
> between builds) then I believe this is a good fix to support the broken
> compile issues reported.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> I'll give the others a chance to provide any feedback if necessary and
> then we can push this into the master branch, and then get buildroot
> package building the latest version.

I was initially a bit worried that we would link to libatomic.so
unnecessarily, or that it would break on platforms not providing
libatomic.so, but as all that seems fine,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Out of curiosity I still would like to know what code in V4L2VideoDevice
generates atomic calls.

> >      cc.find_library('dl'),
> >      libudev,
> >      dependency('threads'),
> >

Hi Laurent,

On 09/09/2019 00:01, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Sep 06, 2019 at 02:50:38PM +0100, Kieran Bingham wrote:
>> On 05/09/2019 20:04, Fabrice Fontaine wrote:
>>> On some architectures, atomic binutils are provided by the libatomic
>>> library from gcc. Linking with libatomic is therefore necessary,
>>> otherwise the build fails with:
>>>
>>> /home/buildroot/autobuild/run/instance-3/output/host/opt/ext-toolchain/bin/../lib/gcc/sparc-buildroot-linux-uclibc/7.4.0/../../../../sparc-buildroot-linux-uclibc/bin/ld: src/libcamera/4ab8042@@camera@sha/v4l2_videodevice.cpp.o: in function `libcamera::V4L2VideoDevice::queueBuffer(libcamera::Buffer*)':
>>> v4l2_videodevice.cpp:(.text+0x1470): undefined reference to `__atomic_fetch_add_4'
>>>
>>> This is often for example the case on sparc v8 32 bits.
>>>
>>> Fixes:
>>>  - http://autobuild.buildroot.org/results/1f0b8338f5f39aa86b9d432598dae2f53c5f7c84
>>>
>>> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
>>> ---
>>>  src/libcamera/meson.build | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>> index c5d8f11..0706a08 100644
>>> --- a/src/libcamera/meson.build
>>> +++ b/src/libcamera/meson.build
>>> @@ -99,6 +99,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()],
>>>  libcamera_sources += version_cpp
>>>  
>>>  libcamera_deps = [
>>> +    cc.find_library('atomic', required: false),
>>
>> Interestingly, this adds the following clear message when I build on x86:
>>
>> Library atomic found: YES
>>
>> So - my system finds 'libatomic' and now explicitly adds it as the
>> dependencies. (I assumed it was a gcc builtin, for it not to be necessary)
>>
>> I wondered if this caused any ill-effect, but actually I believe this is
>> perfectly safe. Thanks to using meson we have reproducible builds, and I
>> can see that both with and without this explicit linking we get
>> identical output binaries for libcamera.so
> 
> Is that because the linker doesn't find any symbol in libatomic.so that
> are required by libcamera, and thus skips it ?

Perhaps, it could likely be that the necessary intrinsics are provided
by GCC so those get used with precedence, and then there's nothing left
to link to libatomic.


>> With that, and seeing the libcamera tests pass (which is not surprising
>> with no binary change - I've ensured that the git sha1 did not change
>> between builds) then I believe this is a good fix to support the broken
>> compile issues reported.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> I'll give the others a chance to provide any feedback if necessary and
>> then we can push this into the master branch, and then get buildroot
>> package building the latest version.
> 
> I was initially a bit worried that we would link to libatomic.so
> unnecessarily, or that it would break on platforms not providing
> libatomic.so, but as all that seems fine,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Out of curiosity I still would like to know what code in V4L2VideoDevice
> generates atomic calls.

In our master branch, nothing in V4L2VideoDevice. That object file now
compiles cleanly..

In master the only link error point on sparc is:

src/libcamera/4ab8042@@camera@sha/message.cpp.o: In function
`libcamera::Message::registerMessageType()':
message.cpp:(.text+0x178): undefined reference to `__atomic_fetch_add_4'
collect2: error: ld returned 1 exit status

Which is due to the following function:

 Message::Type Message::registerMessageType()
 {
        return static_cast<Message::Type>(nextUserType_++);
 }

Thus - I believe this resolution is still required.



Buildroot is currently still building from:

caf25dc5cfd1 ("libcamera: event_dispatcher_poll: Remove struct keyword
from for-range")


At that commit, in V4L2VideoDevice::queueBuffer() we do a:

	if (queuedBuffersCount_++ == 0)
        	fdEvent_->setEnabled(true);

and in V4L2VideoDevice::dequeueBuffer() we have:

        if (--queuedBuffersCount_ == 0)
                fdEvent_->setEnabled(false);

I believe these are the cause of the atomic references.


Now the only issue is that we have picked up other build errors in
master since this commit in regards to O_TMPFILE usage. <sigh>


But I believe that's separate to this issue, and I believe we are good
to apply this fix.


>>>      cc.find_library('dl'),
>>>      libudev,
>>>      dependency('threads'),
>>>
>

Hi Fabrice,

I have updated the commit message to reflect the link error which occurs
on our master branch as opposed to the build issue you have reported
(those lines of affected code have now been removed):

>     src/libcamera/4ab8042@@camera@sha/message.cpp.o: In function `libcamera::Message::registerMessageType()':
>     message.cpp:(.text+0x178): undefined reference to `__atomic_fetch_add_4'
>     collect2: error: ld returned 1 exit status

I've collected Laurent's RB tag, added my own and pushed to master.

Thank you for this fix.

Unfortunately based on my quick cursory testing the master branch will
hit a secondary issue which will prevent compilation on Sparc in regards
to O_TMPFILE.

Is this something you can tackle and fix as well? I've overspent on my
cycles for build-root support so it may take me some time to get back to
this otherwise.

Thanks and regards

Kieran


On 05/09/2019 20:04, Fabrice Fontaine wrote:
> On some architectures, atomic binutils are provided by the libatomic
> library from gcc. Linking with libatomic is therefore necessary,
> otherwise the build fails with:
> 
> /home/buildroot/autobuild/run/instance-3/output/host/opt/ext-toolchain/bin/../lib/gcc/sparc-buildroot-linux-uclibc/7.4.0/../../../../sparc-buildroot-linux-uclibc/bin/ld: src/libcamera/4ab8042@@camera@sha/v4l2_videodevice.cpp.o: in function `libcamera::V4L2VideoDevice::queueBuffer(libcamera::Buffer*)':
> v4l2_videodevice.cpp:(.text+0x1470): undefined reference to `__atomic_fetch_add_4'
> 
> This is often for example the case on sparc v8 32 bits.
> 
> Fixes:
>  - http://autobuild.buildroot.org/results/1f0b8338f5f39aa86b9d432598dae2f53c5f7c84
> 
> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> ---
>  src/libcamera/meson.build | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index c5d8f11..0706a08 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -99,6 +99,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()],
>  libcamera_sources += version_cpp
>  
>  libcamera_deps = [
> +    cc.find_library('atomic', required: false),
>      cc.find_library('dl'),
>      libudev,
>      dependency('threads'),
>

Hello,

Le lun. 9 sept. 2019 à 13:09, Kieran Bingham
<kieran.bingham@ideasonboard.com> a écrit :
>
> Hi Fabrice,
>
> I have updated the commit message to reflect the link error which occurs
> on our master branch as opposed to the build issue you have reported
> (those lines of affected code have now been removed):
>
> >     src/libcamera/4ab8042@@camera@sha/message.cpp.o: In function `libcamera::Message::registerMessageType()':
> >     message.cpp:(.text+0x178): undefined reference to `__atomic_fetch_add_4'
> >     collect2: error: ld returned 1 exit status
>
> I've collected Laurent's RB tag, added my own and pushed to master.
>
> Thank you for this fix.
>
> Unfortunately based on my quick cursory testing the master branch will
> hit a secondary issue which will prevent compilation on Sparc in regards
> to O_TMPFILE.
>
> Is this something you can tackle and fix as well? I've overspent on my
> cycles for build-root support so it may take me some time to get back to
> this otherwise.
OK, I'll try to fix it in the next few days.
>
> Thanks and regards
>
> Kieran
>
>
> On 05/09/2019 20:04, Fabrice Fontaine wrote:
> > On some architectures, atomic binutils are provided by the libatomic
> > library from gcc. Linking with libatomic is therefore necessary,
> > otherwise the build fails with:
> >
> > /home/buildroot/autobuild/run/instance-3/output/host/opt/ext-toolchain/bin/../lib/gcc/sparc-buildroot-linux-uclibc/7.4.0/../../../../sparc-buildroot-linux-uclibc/bin/ld: src/libcamera/4ab8042@@camera@sha/v4l2_videodevice.cpp.o: in function `libcamera::V4L2VideoDevice::queueBuffer(libcamera::Buffer*)':
> > v4l2_videodevice.cpp:(.text+0x1470): undefined reference to `__atomic_fetch_add_4'
> >
> > This is often for example the case on sparc v8 32 bits.
> >
> > Fixes:
> >  - http://autobuild.buildroot.org/results/1f0b8338f5f39aa86b9d432598dae2f53c5f7c84
> >
> > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> > ---
> >  src/libcamera/meson.build | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index c5d8f11..0706a08 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -99,6 +99,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()],
> >  libcamera_sources += version_cpp
> >
> >  libcamera_deps = [
> > +    cc.find_library('atomic', required: false),
> >      cc.find_library('dl'),
> >      libudev,
> >      dependency('threads'),
> >
>
> --
> Regards
> --
> Kieran
Best Regards,

Fabrice