Re: [PATCH 0/2] Work around broken clang. Again.
On Tue, Mar 21, 2023 at 10:46:36AM +0000, Daniel P. Berrangé wrote:
On Mon, Mar 20, 2023 at 03:17:24PM +0100, Michal Prívozník wrote:
> On 3/16/23 15:51, Andrea Bolognani wrote:
> > A thought about VIR_OPTNONE. It seems to me that we'd want to apply
> > this to all the functions that currently are marked with G_NO_INLINE
> > for mocking purposes. So, wouldn't it make sense to instead have a
> > single VIR_MOCKABLE annotation that combines the two, and use that
> > everywhere?
> >
>
> The problem is, __attribute__((optnone)) works only when passed at
> function definition, while noinline can be passed at function
> definition.
Is that a problem in fact ? We only need to suppress inlining when
the calling function is in the same compilation unit as the called
function. Likewise for this return value optimization. So if 'noinline'
annotation was in the definition, rather than the declaration, it
would be fine for our purposes.
I think it would make sense to have a VIR_MOCKABLE annotation that
covered 'noline', 'optnone' and 'noipa' attributes (the precise
set
varying according to what compiler we target).
Empirically 'optnone' does *NOT* fix the problem we face. The
optimizations about return value still get performed with 'optnone'
present :-(
After chasing links I found
https://reviews.llvm.org/D28404#641077
where there is this nugget
"optnone isn't *really* no optimizations: clearly this is true,
but then neither is -O0. We run the always inliner, a couple
of other passes, and we run several parts of the code generators
optimizer. I understand why optnone deficiencies (ie, too many
optimizations) might be frustrating, but having *more users*
seems likely to make this *better*."
Much of that ticket is debate of whether 'optnone' should be thue
same as '-O0'
There is also a blog post
http://maskray.me/blog/2021-05-09-fno-semantic-interposition
which confirms what we discovered
"For ELF -fpic, Clang never suppresses interprocedural optimizations
(including inlining) on default visibility external linkage
definitions. So projects relying on blocked interprocedural
optimizations have been broken for years. They only probably work
recently by specifying -fsemantic-interposition."
By 'default visibility' they basically mean any symbol which is not
marked as 'weak', which is what we tried todo before and then reverted
with:
commit 407a281a8e2b6c5078ba1148535663ea64fd9314
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Wed Jul 12 11:07:17 2017 +0100
Revert "Prevent more compiler optimization of mockable functions"
This reverts commit e4b980c853d2114b25fa805a84ea288384416221.
Looking at the revert again though, I think we might not be out of
luck. We had to revert because when we put stuff into .a files it
got lost when linked to the final executable when it was marked
weak. I think we might be able to work around this using linker
flag -Wl,--whole-archive.
This wasn't practical in 2017 as we still used automake+libtool
and they had a habit of re-ordering such args. Now we're using
meson though, it has a 'link_whole' option integrated which
should do the right thing with -Wl,--whole-archive.
This might let us re-try the weak function approach.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|