[Spice-devel] [PATCH v2 2/2] Avoid clang warnings on casts with stricter alignment requirements

[Christophe de Dinechin]

> On 24 May 2017, at 10:09, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> On Tue, May 23, 2017 at 12:55:13PM +0200, Victor Toso wrote:
>>> 
>>> I only see -DNDEBUG being defined for cmake builds in rpm --showrc, and
>>> none of my 250 spec files have a -DNDEBUG in them, so I would not expect
>>> this to be set. I think we have SPICE_DEBUG already which might make
>>> sense, another alternative would be to have a --enable-alignment-debug,
>> 
>> 
>>> or something like glib (SPICE_XXX_DEBUG=alignment:foo:bar)
>> 
>> Just to point out that we have a bug for this [0] and IMHO it would be
>> the best approach but it would take some effort to have it besides the
>> glib that we would need is 2.50 (current required is 2.36)
>> [0] https://bugs.freedesktop.org/show_bug.cgi?id=91838
> 
> Actually this is not what I had in mind when writing my email, but might
> fit yes.

I don’t think it does for this specific case. What Frediano objected to was the extra runtime cost, which presently on x86 is one testb instruction and one rarely-taken conditional jump. The structured logging facility is neat, but it’s one memory test instruction plus one rarely-taken conditional jump. Memory test is at least as slow as register test, so it’s not a win.

As I understand it, what is being discussed here is how to remove the test entirely, which is a #ifdef. So the choices are:

1) Use the libc standard #ifdef, NDEBUG, used to remove assert() tests from the code. That was my first choice, because it’s standard, known to package maintainers (even if not used much by Red Hat), and foremost because Spice had no such mechanism in place yet, so I decided not to invent one.

2) Use a specific #ifdef that you can pass in CFLAGS. That was my second choice, and I used SPICE_DEBUG_ALIGNMENT. That means only someone knowing about that feature gets to use it, but I think for something really specific and rarely used like this, it’s OK. My concern is bit rot, i.e. the corresponding code may be too rarely tested.

3) Add a configure option for that specific CFLAGS. That’s how I understood Christophe’s suggestion. But when I tried, I ran into issues that we need that both in spice-common and in spice-gtk for now, so there’s a non-negligible chance of a configuration mismatch leading for example to unsatisfied symbols. So after trying, I decided I did not like it and did not send an update with that variant.

For now, my vote goes for 1 (even after taking into account Christophe’s comments), I find 2 acceptable, and I vote against 3 after having tried it.


Regards,
Christophe

> 
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel