| Message ID | 20200307211326.26994-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 07/03/2020 21:13, Laurent Pinchart wrote: > libatomic provides atomic operations that are also available as compiler > built-ins on most platforms. On some platforms (such as Sparc) linking > against libatomic is required to use any atomic operation. libcamera > links against libatomic if available for that reason. > > This however makes libcamera link against libatomic even when not > required, if libatomic is available. Restrict this to only the cases > where libatomic is required with a link test. > Hrm, I thought we discussed and researched this topic when it went in. I thought I recalled testing this and identifying that even if libatomic was found on the system, and would be available to link - The toolchain would take the internals as a preference and caused no binary change in the output. But I won't overly object to this change though. > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > meson.build | 1 + > src/libcamera/meson.build | 2 +- > src/meson.build | 17 +++++++++++++++++ > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/meson.build b/meson.build > index c6e6a934e54e..a1e455249b0d 100644 > --- a/meson.build > +++ b/meson.build > @@ -24,6 +24,7 @@ libcamera_version = libcamera_git_version.split('+')[0] > , > # Configure the build environment. > cc = meson.get_compiler('c') > +cxx = meson.get_compiler('cpp') > config_h = configuration_data() > > if cc.has_header_symbol('execinfo.h', 'backtrace') > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index 1b69bc0dee54..ac6f597c6188 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -93,7 +93,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()], > libcamera_sources += version_cpp > > libcamera_deps = [ > - cc.find_library('atomic', required: false), > + libatomic_dep, > cc.find_library('dl'), > libudev, > dependency('threads'), > diff --git a/src/meson.build b/src/meson.build > index d818d8b86d93..abf755936f2b 100644 > --- a/src/meson.build > +++ b/src/meson.build Hrm ... is this the right place for this? I guess it's not a 'bad' place for it. Our top level meson.build is getting busier and busier... and this presumably affects only 'src' code (hence being under src) ... but then 'test/' misses out? Still I guess test/ may be less likely to use atomics... > @@ -1,3 +1,20 @@ > +# On some platforms gcc doesn't provide built-ins for atomic operations, > +# explicit linking against libatomic is required. > +if cxx.links(''' > + #include <atomic> > + #include <cstdint> > + int main(void) > + { > + std::atomic<std::uint32_t> u32(0); > + std::atomic<std::uint64_t> u64(0); > + return u32.load() + u64.load(); > + } > +''') > + libatomic_dep = dependency('', required : false) > +else > + libatomic_dep = cc.find_library('atomic') > +endif > + > if get_option('android') > subdir('android') > endif >
Hi Kieran, On Sat, Mar 07, 2020 at 11:01:06PM +0000, Kieran Bingham wrote: > On 07/03/2020 21:13, Laurent Pinchart wrote: > > libatomic provides atomic operations that are also available as compiler > > built-ins on most platforms. On some platforms (such as Sparc) linking > > against libatomic is required to use any atomic operation. libcamera > > links against libatomic if available for that reason. > > > > This however makes libcamera link against libatomic even when not > > required, if libatomic is available. Restrict this to only the cases > > where libatomic is required with a link test. > > Hrm, I thought we discussed and researched this topic when it went in. > > I thought I recalled testing this and identifying that even if libatomic > was found on the system, and would be available to link - The toolchain > would take the internals as a preference and caused no binary change in > the output. > > But I won't overly object to this change though. You're right, I checked again: --- libcamera.so.1.hex 2020-03-08 01:25:05.609314371 +0200 +++ libcamera.so.2.hex 2020-03-08 01:25:13.060315088 +0200 @@ -35886,7 +35886,7 @@ 0008c600 6f 6e 00 00 4d 61 6e 75 61 6c 45 78 70 6f 73 75 |on..ManualExposu| 0008c610 72 65 00 00 4d 61 6e 75 61 6c 47 61 69 6e 00 00 |re..ManualGain..| 0008c620 4c 6f 63 61 74 69 6f 6e 00 00 00 00 76 30 2e 30 |Location....v0.0| -0008c630 2e 30 2b 31 31 31 36 2d 38 36 32 33 31 38 36 38 |.0+1116-86231868| +0008c630 2e 30 2b 31 31 31 37 2d 33 63 36 37 62 33 34 34 |.0+1117-3c67b344| 0008c640 00 00 00 00 42 75 66 66 65 72 00 00 53 65 72 69 |....Buffer..Seri| 0008c650 61 6c 69 7a 61 74 69 6f 6e 00 00 00 2e 2e 2f 2e |alization...../.| 0008c660 2e 2f 73 72 63 2f 6c 69 62 63 61 6d 65 72 61 2f |./src/libcamera/| I'll drop this patch. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > meson.build | 1 + > > src/libcamera/meson.build | 2 +- > > src/meson.build | 17 +++++++++++++++++ > > 3 files changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/meson.build b/meson.build > > index c6e6a934e54e..a1e455249b0d 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -24,6 +24,7 @@ libcamera_version = libcamera_git_version.split('+')[0] > > , > > # Configure the build environment. > > cc = meson.get_compiler('c') > > +cxx = meson.get_compiler('cpp') > > config_h = configuration_data() > > > > if cc.has_header_symbol('execinfo.h', 'backtrace') > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index 1b69bc0dee54..ac6f597c6188 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -93,7 +93,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()], > > libcamera_sources += version_cpp > > > > libcamera_deps = [ > > - cc.find_library('atomic', required: false), > > + libatomic_dep, > > cc.find_library('dl'), > > libudev, > > dependency('threads'), > > diff --git a/src/meson.build b/src/meson.build > > index d818d8b86d93..abf755936f2b 100644 > > --- a/src/meson.build > > +++ b/src/meson.build > > Hrm ... is this the right place for this? > > I guess it's not a 'bad' place for it. > Our top level meson.build is getting busier and busier... and this > presumably affects only 'src' code (hence being under src) ... but then > 'test/' misses out? > > Still I guess test/ may be less likely to use atomics... test/meson.build is included after src/meson.build, so libatomic_dep is available for it. > > @@ -1,3 +1,20 @@ > > +# On some platforms gcc doesn't provide built-ins for atomic operations, > > +# explicit linking against libatomic is required. > > +if cxx.links(''' > > + #include <atomic> > > + #include <cstdint> > > + int main(void) > > + { > > + std::atomic<std::uint32_t> u32(0); > > + std::atomic<std::uint64_t> u64(0); > > + return u32.load() + u64.load(); > > + } > > +''') > > + libatomic_dep = dependency('', required : false) > > +else > > + libatomic_dep = cc.find_library('atomic') > > +endif > > + > > if get_option('android') > > subdir('android') > > endif
Hi Kieran, On Sun, Mar 08, 2020 at 01:27:14AM +0200, Laurent Pinchart wrote: > On Sat, Mar 07, 2020 at 11:01:06PM +0000, Kieran Bingham wrote: > > On 07/03/2020 21:13, Laurent Pinchart wrote: > > > libatomic provides atomic operations that are also available as compiler > > > built-ins on most platforms. On some platforms (such as Sparc) linking > > > against libatomic is required to use any atomic operation. libcamera > > > links against libatomic if available for that reason. > > > > > > This however makes libcamera link against libatomic even when not > > > required, if libatomic is available. Restrict this to only the cases > > > where libatomic is required with a link test. > > > > Hrm, I thought we discussed and researched this topic when it went in. > > > > I thought I recalled testing this and identifying that even if libatomic > > was found on the system, and would be available to link - The toolchain > > would take the internals as a preference and caused no binary change in > > the output. > > > > But I won't overly object to this change though. > > You're right, I checked again: > > --- libcamera.so.1.hex 2020-03-08 01:25:05.609314371 +0200 > +++ libcamera.so.2.hex 2020-03-08 01:25:13.060315088 +0200 > @@ -35886,7 +35886,7 @@ > 0008c600 6f 6e 00 00 4d 61 6e 75 61 6c 45 78 70 6f 73 75 |on..ManualExposu| > 0008c610 72 65 00 00 4d 61 6e 75 61 6c 47 61 69 6e 00 00 |re..ManualGain..| > 0008c620 4c 6f 63 61 74 69 6f 6e 00 00 00 00 76 30 2e 30 |Location....v0.0| > -0008c630 2e 30 2b 31 31 31 36 2d 38 36 32 33 31 38 36 38 |.0+1116-86231868| > +0008c630 2e 30 2b 31 31 31 37 2d 33 63 36 37 62 33 34 34 |.0+1117-3c67b344| > 0008c640 00 00 00 00 42 75 66 66 65 72 00 00 53 65 72 69 |....Buffer..Seri| > 0008c650 61 6c 69 7a 61 74 69 6f 6e 00 00 00 2e 2e 2f 2e |alization...../.| > 0008c660 2e 2f 73 72 63 2f 6c 69 62 63 61 6d 65 72 61 2f |./src/libcamera/| > > I'll drop this patch. By the way I got the idea from https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/70303d211886ae109dfc0a88948c362359bdd07c. I wonder if it means they're doing it wrong too. > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > --- > > > meson.build | 1 + > > > src/libcamera/meson.build | 2 +- > > > src/meson.build | 17 +++++++++++++++++ > > > 3 files changed, 19 insertions(+), 1 deletion(-) > > > > > > diff --git a/meson.build b/meson.build > > > index c6e6a934e54e..a1e455249b0d 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -24,6 +24,7 @@ libcamera_version = libcamera_git_version.split('+')[0] > > > , > > > # Configure the build environment. > > > cc = meson.get_compiler('c') > > > +cxx = meson.get_compiler('cpp') > > > config_h = configuration_data() > > > > > > if cc.has_header_symbol('execinfo.h', 'backtrace') > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > index 1b69bc0dee54..ac6f597c6188 100644 > > > --- a/src/libcamera/meson.build > > > +++ b/src/libcamera/meson.build > > > @@ -93,7 +93,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()], > > > libcamera_sources += version_cpp > > > > > > libcamera_deps = [ > > > - cc.find_library('atomic', required: false), > > > + libatomic_dep, > > > cc.find_library('dl'), > > > libudev, > > > dependency('threads'), > > > diff --git a/src/meson.build b/src/meson.build > > > index d818d8b86d93..abf755936f2b 100644 > > > --- a/src/meson.build > > > +++ b/src/meson.build > > > > Hrm ... is this the right place for this? > > > > I guess it's not a 'bad' place for it. > > Our top level meson.build is getting busier and busier... and this > > presumably affects only 'src' code (hence being under src) ... but then > > 'test/' misses out? > > > > Still I guess test/ may be less likely to use atomics... > > test/meson.build is included after src/meson.build, so libatomic_dep is > available for it. > > > > @@ -1,3 +1,20 @@ > > > +# On some platforms gcc doesn't provide built-ins for atomic operations, > > > +# explicit linking against libatomic is required. > > > +if cxx.links(''' > > > + #include <atomic> > > > + #include <cstdint> > > > + int main(void) > > > + { > > > + std::atomic<std::uint32_t> u32(0); > > > + std::atomic<std::uint64_t> u64(0); > > > + return u32.load() + u64.load(); > > > + } > > > +''') > > > + libatomic_dep = dependency('', required : false) > > > +else > > > + libatomic_dep = cc.find_library('atomic') > > > +endif > > > + > > > if get_option('android') > > > subdir('android') > > > endif
diff --git a/meson.build b/meson.build index c6e6a934e54e..a1e455249b0d 100644 --- a/meson.build +++ b/meson.build @@ -24,6 +24,7 @@ libcamera_version = libcamera_git_version.split('+')[0] # Configure the build environment. cc = meson.get_compiler('c') +cxx = meson.get_compiler('cpp') config_h = configuration_data() if cc.has_header_symbol('execinfo.h', 'backtrace') diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index 1b69bc0dee54..ac6f597c6188 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -93,7 +93,7 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()], libcamera_sources += version_cpp libcamera_deps = [ - cc.find_library('atomic', required: false), + libatomic_dep, cc.find_library('dl'), libudev, dependency('threads'), diff --git a/src/meson.build b/src/meson.build index d818d8b86d93..abf755936f2b 100644 --- a/src/meson.build +++ b/src/meson.build @@ -1,3 +1,20 @@ +# On some platforms gcc doesn't provide built-ins for atomic operations, +# explicit linking against libatomic is required. +if cxx.links(''' + #include <atomic> + #include <cstdint> + int main(void) + { + std::atomic<std::uint32_t> u32(0); + std::atomic<std::uint64_t> u64(0); + return u32.load() + u64.load(); + } +''') + libatomic_dep = dependency('', required : false) +else + libatomic_dep = cc.find_library('atomic') +endif + if get_option('android') subdir('android') endif
libatomic provides atomic operations that are also available as compiler built-ins on most platforms. On some platforms (such as Sparc) linking against libatomic is required to use any atomic operation. libcamera links against libatomic if available for that reason. This however makes libcamera link against libatomic even when not required, if libatomic is available. Restrict this to only the cases where libatomic is required with a link test. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- meson.build | 1 + src/libcamera/meson.build | 2 +- src/meson.build | 17 +++++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-)