[PATCH] fix asm when strict ISO c and ensure dix-config.h is included first

Matt Turner mattst88 at gmail.com
Wed Nov 18 16:59:52 PST 2015


On Mon, Nov 16, 2015 at 10:03 PM, Richard PALO <richard at netbsd.org> wrote:
> From: Richard PALO <richard at NetBSD.org>
>
> The following is for the source found in the glamor subdirectory:
>
> Define a macro 'asm' for the case of a strict ISO gnu c compatible compiler,
> (NB this may need to be revised in case of being compiled in by c++)
>
> Also, ensure that dix-config.h is effectively the first included file in
> various glamor modules such that feature tests (at least on SunOS i386)
> is correctly instrumented (such as _LARGEFILE_SOURCE/_FILE_OFFSET_BITS
> as well as POSIX profile and other extensions).
>
> Since glamor_priv.h correctly includes dix-config.h, the easiest way is
> to simply adjust its position in the affected .c files.
>
> Signed-off-by: Richard PALO <richard at NetBSD.org>
> ---
>  glamor/glamor.c                  | 2 +-
>  glamor/glamor_composite_glyphs.c | 2 +-
>  glamor/glamor_core.c             | 3 +--
>  glamor/glamor_fbo.c              | 3 +--
>  glamor/glamor_largepixmap.c      | 3 +--
>  glamor/glamor_picture.c          | 2 +-
>  glamor/glamor_pixmap.c           | 2 +-
>  glamor/glamor_utils.h            | 4 ++++
>  8 files changed, 11 insertions(+), 10 deletions(-)

This is all because of

static inline unsigned long
__fls(unsigned long x)
{
 asm("bsr %1,%0":"=r"(x)
 :     "rm"(x));
    return x;
}

There's no reason to use inline assembly there and in fact it's likely
harmful. Also, bsr has unspecified behavior for an input of zero, so
this implementation doesn't even match the non-assembly
implementation.

Just change the implementation to

   return n == 0 ? 0 : 32 - __builtin_clz(n);

and get rid of the other definition of __fls() immediately below.

__builtin_ctz has been supported by gcc since v3.4 so it should be
safe to use anywhere gcc inline assembly is safe to use.


More information about the xorg-devel mailing list