Proposed patch to support clang compilation of util-image

Submitted by Pablo Cholaky on Oct. 8, 2016, 7:58 p.m.

Details

Message ID CAD_UamWrvqE8bwOx-65sE=bV_hC7UaOu90v2NTWCb_+f3uec-g@mail.gmail.com
State New
Headers show
Series "Proposed patch to support clang compilation of util-image" ( rev: 1 ) in XCB

Not browsing as part of any series.

Commit Message

Pablo Cholaky Oct. 8, 2016, 7:58 p.m.
Guys,

I think this patch is currently harmless and makes support to clang to
properly compile it.

Author: Pablo Cholaky <waltercool@slash.cl>
Date:   Sat Oct 8 16:52:37 2016 -0300

   Added return to xcb_host_byte_order function

   Signed-off-by: Pablo Cholaky <waltercool@slash.cl>


Using clang it fails just after configure

Making all in image
make[2]: Entering directory
'/var/tmp/portage/x11-libs/xcb-util-image-0.4.0/work/xcb-util-image-0.4.0-abi_x86_64.amd64/image'
/bin/sh ../libtool  --tag=CC   --mode=compile clang -DHAVE_CONFIG_H -I.
-I/var/tmp/portage/x11-libs/xcb-util-image-0.4.0/work/xcb-util-image-0.4.0/image
-I..       -Wall -Wpointer-arith -Wmissing-declarations -Wformat=2
-Wstrict-prototypes -Wmissing-prototypes -Wnested-externs
-Wbad-function-cast -Wold-style-definition -Wdeclaration-after-statement
-Wunused -Wuninitialized -Wshadow -Wmissing-noreturn
-Wmissing-format-attribute -Wredundant-decls -Werror=implicit
-Werror=nonnull -Werror=init-self -Werror=main -Werror=missing-braces
-Werror=sequence-point -Werror=return-type -Werror=trigraphs
-Werror=array-bounds -Werror=write-strings -Werror=address
-Werror=int-to-pointer-cast -Werror=pointer-to-int-cast
-fno-strict-aliasing -O2 -pipe -g -c -o xcb_image.lo
/var/tmp/portage/x11-libs/xcb-util-image-0.4.0/work/xcb-util-image-0.4.0/image/xcb_image.c
libtool: compile:  clang -DHAVE_CONFIG_H -I.
-I/var/tmp/portage/x11-libs/xcb-util-image-0.4.0/work/xcb-util-image-0.4.0/image
-I.. -Wall -Wpointer-arith -Wmissing-declarations -Wformat=2
-Wstrict-prototypes -Wmissing-prototypes -Wnested-externs
-Wbad-function-cast -Wold-style-definition -Wdeclaration-after-statement
-Wunused -Wuninitialized -Wshadow -Wmissing-noreturn
-Wmissing-format-attribute -Wredundant-decls -Werror=implicit
-Werror=nonnull -Werror=init-self -Werror=main -Werror=missing-braces
-Werror=sequence-point -Werror=return-type -Werror=trigraphs
-Werror=array-bounds -Werror=write-strings -Werror=address
-Werror=int-to-pointer-cast -Werror=pointer-to-int-cast
-fno-strict-aliasing -O2 -pipe -g -c
/var/tmp/portage/x11-libs/xcb-util-image-0.4.0/work/xcb-util-image-0.4.0/image/xcb_image.c
 -fPIC -DPIC -o .libs/xcb_image.o
In file included from
/var/tmp/portage/x11-libs/xcb-util-image-0.4.0/work/xcb-util-image-0.4.0/image/xcb_image.c:37:
/var/tmp/portage/x11-libs/xcb-util-image-0.4.0/work/xcb-util-image-0.4.0/image/xcb_bitops.h:210:1:
error: control may reach end of non-void function [-Werror,-Wreturn-type]
}
^
1 error generated.

Based on the patch initially proposed here:
https://patchwork.openembedded.org/patch/118073/ and discussed here
https://github.com/gentoo/gentoo/pull/2496 and here
https://github.com/gentoo/musl/pull/12

Regards.

Patch hide | download patch | download mbox

diff --git a/image/xcb_bitops.h b/image/xcb_bitops.h
index a6872a1..bf6fdc1 100644
--- a/image/xcb_bitops.h
+++ b/image/xcb_bitops.h
@@ -207,6 +207,7 @@  xcb_host_byte_order(void) {
      return XCB_IMAGE_ORDER_LSB_FIRST;
  }
  assert(0);
+  return -1;
}

#endif /* __XCB_BITOPS_H__ */

Comments

On 10/ 8/16 12:58 PM, Pablo Cholaky wrote:
> I think this patch is currently harmless and makes support to clang to properly
> compile it.

clang doesn't complain on the current code for me (probably because my libc 
headers define the function called on a failed assert with the noreturn
attribute), but adding this:

>   assert(0);
> +  return -1;

causes clang to report two new warnings for me:

./xcb_bitops.h:210:10: warning: integer constant not in range of
       enumerated type 'xcb_image_order_t' (aka 'enum xcb_image_order_t')
       [-Wassign-enum]
   return -1;
          ^
./xcb_bitops.h:210:10: warning: implicit conversion changes signedness:
       'int' to 'xcb_image_order_t' (aka 'enum xcb_image_order_t')
       [-Wsign-conversion]
   return -1;
   ~~~~~~ ^~

Admittedly I run clang with more warnings enabled than the default build, to
help find issues during development.
Yeah, assert() should be marked noreturn (directly or indirectly) in
general or bad things happen. Those assert()s really want to be assert()s
or calls to abort() or something: there is nothing good that can happen
after returning from those places.

On Sun, Oct 16, 2016 at 11:19 PM Alan Coopersmith <
alan.coopersmith@oracle.com> wrote:

> On 10/ 8/16 12:58 PM, Pablo Cholaky wrote:
> > I think this patch is currently harmless and makes support to clang to
> properly
> > compile it.
>
> clang doesn't complain on the current code for me (probably because my libc
> headers define the function called on a failed assert with the noreturn
> attribute), but adding this:
>
> >   assert(0);
> > +  return -1;
>
> causes clang to report two new warnings for me:
>
> ./xcb_bitops.h:210:10: warning: integer constant not in range of
>        enumerated type 'xcb_image_order_t' (aka 'enum xcb_image_order_t')
>        [-Wassign-enum]
>    return -1;
>           ^
> ./xcb_bitops.h:210:10: warning: implicit conversion changes signedness:
>        'int' to 'xcb_image_order_t' (aka 'enum xcb_image_order_t')
>        [-Wsign-conversion]
>    return -1;
>    ~~~~~~ ^~
>
> Admittedly I run clang with more warnings enabled than the default build,
> to
> help find issues during development.
>
> --
>         -Alan Coopersmith-              alan.coopersmith@oracle.com
>          Oracle Solaris Engineering - http://blogs.oracle.com/alanc
> _______________________________________________
> Xcb mailing list
> Xcb@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/xcb
On Mon, Oct 17, 2016 at 07:44:09AM +0000, Bart Massey wrote:
> Yeah, assert() should be marked noreturn (directly or indirectly) in
> general or bad things happen. Those assert()s really want to be assert()s
> or calls to abort() or something: there is nothing good that can happen
> after returning from those places.

I just checked, and clang definitely does support the noreturn
attribute.  And the system headers typically define assert with the
noreturn attribute.

Did the build occur with NDEBUG defined or similar, to no-op out the
assert?
On Mon, Oct 17, 2016 at 3:15 PM, Josh Triplett <josh@joshtriplett.org>
wrote:

> On Mon, Oct 17, 2016 at 07:44:09AM +0000, Bart Massey wrote:
> > Yeah, assert() should be marked noreturn (directly or indirectly) in
> > general or bad things happen. Those assert()s really want to be assert()s
> > or calls to abort() or something: there is nothing good that can happen
> > after returning from those places.
>
> I just checked, and clang definitely does support the noreturn
> attribute.  And the system headers typically define assert with the
> noreturn attribute.
>
> Did the build occur with NDEBUG defined or similar, to no-op out the
> assert?
>

Guys, I'm really sorry of being late responding this, the problem was
caused by musl and not clang nor xcb-image, so please ignore those changes.

Reference:
http://git.musl-libc.org/cgit/musl/commit/?id=e738b8cbe64b6dd3ed9f47b6d4cd7eb2c422b38d

Regards!