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

[Kieran Bingham]

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 at 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 at 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 at 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 at 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 at 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'),
>>>
> 

-- 
Regards
--
Kieran