libusb: add stdio.h header #1327

neheb · 2023-10-16T22:40:12Z

Needed for printf functions. Not always included implicitly.

Youw

I'd say that relying on implicit behavior in general is one of the sources of eternal evil.

Youw · 2023-10-16T23:11:55Z

Out of curiosity: which compiler did catch this (or what was special about the build environment otherwise)?

neheb · 2023-10-16T23:14:01Z

MinGW specifically

mcuee · 2023-10-16T23:59:57Z

MinGW specifically

Just wondering which version of MinGW? Take note we do not really support MinGW.org 32bit compiler (but minor PR will be reviewed and accepted if appropriate).

We recommend MSYS2 distribution of mingw32/mingw64 (and accept PRs to improve MSYS2 environment like ucrt64, clang32, clang64 and clangarm64).

https://www.msys2.org/docs/environments/

Name	Prefix	Toolchain	Architecture	C Library	C++ Library
UCRT64	/ucrt64	gcc	x86_64	ucrt	libstdc++
CLANG64	/clang64	llvm	x86_64	ucrt	libc++
CLANGARM64	/clangarm64	llvm	aarch64	ucrt	libc++
CLANG32	/clang32	llvm	i686	ucrt	libc++
MINGW64	/mingw64	gcc	x86_64	msvcrt	libstdc++
MINGW32	/mingw32	gcc	i686	msvcrt	libstdc++

neheb · 2023-10-17T00:12:58Z

Ubuntu MinGW 64 cross compile.

mcuee · 2023-10-17T00:35:19Z

Ubuntu MinGW 64 cross compile.

I see. Then this PR is good to me.

tormodvolden · 2023-10-17T16:16:16Z

I also always cross-compile on Ubuntu (20.04, gcc 9.3) or Debian (gcc 12.2). But I don't see the warning. Do you add extra flags? Build log extract?

Youw · 2023-10-17T16:18:42Z

I'm pretty sure this is compiler-version specific. I've seen it multiple times when after a compiler upgrade some of the standard headers are no longer implicitly inlcluded - and that is perfectly in align with The Standard.

tormodvolden · 2023-10-17T16:43:02Z

@Youw That's why I find "Ubuntu MinGW 64" too unprecise. Anyway, my questions to @neheb remain.

Another thing, I am not sure the one in tests/libusb_testlib.h is useful.

tormodvolden · 2023-10-17T16:50:44Z

And if it is needed in libusb/libusbi.h if ENABLE_LOGGING is not set?

I need to understand this better before I can approve the patch.

neheb · 2023-10-17T19:00:23Z

Caught me.

So the real motivation for this change is to try to fix the following warnings:

In file included from ../subprojects/libusb-1.0.26/libusb/core.c:23:
../subprojects/libusb-1.0.26/libusb/core.c: In function ‘libusb_wrap_sys_device’:
../subprojects/libusb-1.0.26/libusb/core.c:1249:16: warning: unknown conversion type character ‘l’ in format [-Wformat=]
 1249 |  usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR, (uintptr_t)sys_dev);
      |                ^~~~~~~~~~~~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1249:2: note: in expansion of macro ‘usbi_dbg’
 1249 |  usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR, (uintptr_t)sys_dev);
      |  ^~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1249:16: warning: too many arguments for format [-Wformat-extra-args]
 1249 |  usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR, (uintptr_t)sys_dev);
      |                ^~~~~~~~~~~~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1249:2: note: in expansion of macro ‘usbi_dbg’
 1249 |  usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR, (uintptr_t)sys_dev);
      |  ^~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1264:17: warning: unknown conversion type character ‘l’ in format [-Wformat=]
 1264 |   usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR " returns %d", (uintptr_t)sys_dev, r);
      |                 ^~~~~~~~~~~~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1264:3: note: in expansion of macro ‘usbi_dbg’
 1264 |   usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR " returns %d", (uintptr_t)sys_dev, r);
      |   ^~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1264:17: warning: format ‘%d’ expects argument of type ‘int’, but argument 5 has type ‘long long unsigned int’ [-Wformat=]
 1264 |   usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR " returns %d", (uintptr_t)sys_dev, r);
      |                 ^~~~~~~~~~~~~~~~~~~~~                        ~~~~~~~~~~~~~~~~~~
      |                                                              |
      |                                                              long long unsigned int
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1264:3: note: in expansion of macro ‘usbi_dbg’
 1264 |   usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR " returns %d", (uintptr_t)sys_dev, r);
      |   ^~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1264:17: warning: too many arguments for format [-Wformat-extra-args]
 1264 |   usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR " returns %d", (uintptr_t)sys_dev, r);
      |                 ^~~~~~~~~~~~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/core.c:1264:3: note: in expansion of macro ‘usbi_dbg’
 1264 |   usbi_dbg(ctx, "wrap_sys_device 0x%" PRIxPTR " returns %d", (uintptr_t)sys_dev, r);
      |   ^~~~~~~~
[22/33] Compiling C object subprojects/libusb-1.0.26/libusb-1.0.dll.p/libusb_os_windows_winusb.c.obj
In file included from ../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:33:
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c: In function ‘_hid_get_report’:
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:3594:30: warning: unknown conversion type character ‘l’ in format [-Wformat=]
 3594 |   usbi_warn(DEVICE_CTX(dev), "invalid size (%"PRIuPTR")", (uintptr_t)size);
      |                              ^~~~~~~~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:3594:3: note: in expansion of macro ‘usbi_warn’
 3594 |   usbi_warn(DEVICE_CTX(dev), "invalid size (%"PRIuPTR")", (uintptr_t)size);
      |   ^~~~~~~~~
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:3594:30: warning: too many arguments for format [-Wformat-extra-args]
 3594 |   usbi_warn(DEVICE_CTX(dev), "invalid size (%"PRIuPTR")", (uintptr_t)size);
      |                              ^~~~~~~~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:3594:3: note: in expansion of macro ‘usbi_warn’
 3594 |   usbi_warn(DEVICE_CTX(dev), "invalid size (%"PRIuPTR")", (uintptr_t)size);
      |   ^~~~~~~~~
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c: In function ‘_hid_set_report’:
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:3648:30: warning: unknown conversion type character ‘l’ in format [-Wformat=]
 3648 |   usbi_warn(DEVICE_CTX(dev), "invalid size (%"PRIuPTR")", (uintptr_t)size);
      |                              ^~~~~~~~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:3648:3: note: in expansion of macro ‘usbi_warn’
 3648 |   usbi_warn(DEVICE_CTX(dev), "invalid size (%"PRIuPTR")", (uintptr_t)size);
      |   ^~~~~~~~~
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:3648:30: warning: too many arguments for format [-Wformat-extra-args]
 3648 |   usbi_warn(DEVICE_CTX(dev), "invalid size (%"PRIuPTR")", (uintptr_t)size);
      |                              ^~~~~~~~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/libusbi.h:315:67: note: in definition of macro ‘_usbi_log’
  315 | #define _usbi_log(ctx, level, ...) usbi_log(ctx, level, __func__, __VA_ARGS__)
      |                                                                   ^~~~~~~~~~~
../subprojects/libusb-1.0.26/libusb/os/windows_winusb.c:3648:3: note: in expansion of macro ‘usbi_warn’
 3648 |   usbi_warn(DEVICE_CTX(dev), "invalid size (%"PRIuPTR")", (uintptr_t)size);
      |   ^~~~~~~~~

The issue is that MinGW has a macro defined in stdio.h which switches between printf and gnu_printf. Example of how a different project handles this:

https://github.com/Exiv2/exiv2/blob/main/src/image_int.hpp#L15

This PR was meant as a first step to fixing that.

tormodvolden · 2023-10-17T20:11:47Z

So if we do like exiv and conditionally use __MINGW_PRINTF_FORMAT the warnings go away?

Which version of gcc are you using? Do you use non-default configuration options or flags?

neheb · 2023-10-17T20:26:05Z

x86_64-w64-mingw32-g++ --version
x86_64-w64-mingw32-g++ (GCC) 10-posix 20220113

and yes. currently the printf attribute is set with configure.ac, which would have to be modified.

neheb · 2023-10-18T02:10:35Z

added configure.ac change thanks to ChatGPT. I don't really speak Autotools.

neheb · 2023-10-18T02:32:05Z

I have no idea why there are 4 different build systems here. Sounds horrible to maintain.

Youw · 2023-10-19T07:01:53Z

libusb/libusbi.h

@@ -30,6 +30,7 @@
 #include <inttypes.h>
 #include <stdarg.h>
 #include <stddef.h>
+#include <stdio.h>


with the chanes to configure.ac do we still need these?

any comments?

yes this is still needed.

tormodvolden · 2023-10-19T08:18:09Z

Because of the unclear copyright situation I am not comfortable with merging stuff coming straight out of ChatGPT or code copilot etc (as long as I know it, which I of course cannot unless people are frank about it) into a GPL project. To "clean room" design this (I haven't looked at the suggested change), can someone explain what needs to be done in configure.ac and I'll try to implement it and compare afterwards? Or someone who did look at it confirm it is 100% trivial and copyright safe.

neheb · 2023-10-19T17:09:58Z

It’s not a direct copy. It’s changed from the original output, as most things have to be with ChatGPT.

edit: anyway, check if __MINGW_PRINTF_FORMAT is defined and if it is, use it for PRINTF_FORMAT.

tormodvolden · 2023-11-05T15:46:17Z

Such a test could be something along:

@@ -414,6 +414,13 @@ if test "x$backend" = xwindows; then
        CFLAGS="${saved_CFLAGS}"
 fi
 
+dnl MinGW being awkward here
+if test "x$backend" = xwindows; then
+       AC_CHECK_DECL([__MINGW_PRINTF_FORMAT],
+               [AC_DEFINE([PRINTF_FORMAT(a, b)], [__attribute__ ((__format__ (__MINGW_PRINTF_FORMAT, a, b)))], [Define to the attribute for enabling parameter checks on printf-like functions.])]
+               [], [[#include <stdio.h>]])
+fi
+
 SHARED_CFLAGS="-Wall -Wextra -Wshadow -Wunused -Wwrite-strings -Werror=format-security -Werror=implicit-function-declaration -Werror=implicit-int -Werror=init-self -Werror=missing-prototypes -Werror=strict-prototypes -Werror=undef -Werror=uninitialized"
 
 AM_CPPFLAGS="${EXTRA_CPPFLAGS}"

But I am not sure this is the correct way to deal with the issue, even if it would work in your case. The __MINGW_PRINTF_FORMAT macro is something internal to MinGW and not something we should have to deal with. If this is a needed workaround for a known bug in MinGW, it could be considered, but then we have to know more about this.

What is needed to reproduce your problem?

neheb · 2023-11-05T16:19:05Z

This macro is needed as libusb implements its own formatting functions based on printf. I posted the compilation warnings earlier.

neheb · 2023-11-21T00:45:44Z

anything else missing here?

Youw · 2023-11-21T09:22:11Z

#1327 (comment)

Using __MINGW_PRINTF_FORMAT, which is what the headers use. add stdio.h header to get it defined. Needed for printf functions. Not always included implicitly. Signed-off-by: Rosen Penev <rosenp@gmail.com>

tormodvolden · 2023-11-27T21:25:08Z

I think there is not enough explanation here. Please don't be afraid of being verbose and adding context, background and examples. I think you are using some special flags, but you have not mentioned which and why.

My remarks above remain. E.g. stdio.h is conditionally included in libusbi.h

Also, can this be related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95130 or https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-gcc/0300-override-builtin-printf-format.patch ?

neheb · 2023-11-27T21:56:47Z

There are no special flags being passed. Context is that if -Wformat is passed, these warnings show up.

Those links seem unrelated.

hjelmn · 2023-11-28T21:13:31Z

Not comfortable including any ML-generated code in this project. The original source is unknown and it is possible it does not comply with our licensing or libusb may not comply with it. ChatGPT is less AI than it is a huge copyright infringement tool :p.

That aside, why are we using m4 to define this instead of just defining it in the header where it is used? That removes the need for any m4 changes.

hjelmn · 2023-11-28T21:31:46Z

Opened #1352 to remove the unnecessary m4. Should make it easier to write your patch.

Youw approved these changes Oct 16, 2023

View reviewed changes

mcuee added the Misc Whitespace, typo, coding style, etc label Oct 16, 2023

neheb force-pushed the form branch from 5b4e8ff to 6b6d4d2 Compare October 18, 2023 02:09

neheb force-pushed the form branch from 6b6d4d2 to 48c3117 Compare October 18, 2023 02:26

Youw reviewed Oct 19, 2023

View reviewed changes

Youw mentioned this pull request Oct 23, 2023

Fix MinGW CI build libusb/hidapi#632

Closed

libusb: fix MinGW format warnings

d47944e

Using __MINGW_PRINTF_FORMAT, which is what the headers use. add stdio.h header to get it defined. Needed for printf functions. Not always included implicitly. Signed-off-by: Rosen Penev <rosenp@gmail.com>

neheb force-pushed the form branch from 48c3117 to d47944e Compare November 21, 2023 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libusb: add stdio.h header #1327

libusb: add stdio.h header #1327

neheb commented Oct 16, 2023

Youw left a comment

Youw commented Oct 16, 2023 •

edited

neheb commented Oct 16, 2023

mcuee commented Oct 16, 2023 •

edited

neheb commented Oct 17, 2023

mcuee commented Oct 17, 2023

tormodvolden commented Oct 17, 2023 •

edited

Youw commented Oct 17, 2023

tormodvolden commented Oct 17, 2023

tormodvolden commented Oct 17, 2023 •

edited

neheb commented Oct 17, 2023

tormodvolden commented Oct 17, 2023

neheb commented Oct 17, 2023

neheb commented Oct 18, 2023

neheb commented Oct 18, 2023

Youw Oct 19, 2023

Youw Nov 21, 2023

neheb Nov 21, 2023

tormodvolden commented Oct 19, 2023

neheb commented Oct 19, 2023 •

edited

tormodvolden commented Nov 5, 2023

neheb commented Nov 5, 2023

neheb commented Nov 21, 2023

Youw commented Nov 21, 2023

tormodvolden commented Nov 27, 2023

neheb commented Nov 27, 2023

hjelmn commented Nov 28, 2023

hjelmn commented Nov 28, 2023

libusb: add stdio.h header #1327

Are you sure you want to change the base?

libusb: add stdio.h header #1327

Conversation

neheb commented Oct 16, 2023

Youw left a comment

Choose a reason for hiding this comment

Youw commented Oct 16, 2023 • edited

neheb commented Oct 16, 2023

mcuee commented Oct 16, 2023 • edited

neheb commented Oct 17, 2023

mcuee commented Oct 17, 2023

tormodvolden commented Oct 17, 2023 • edited

Youw commented Oct 17, 2023

tormodvolden commented Oct 17, 2023

tormodvolden commented Oct 17, 2023 • edited

neheb commented Oct 17, 2023

tormodvolden commented Oct 17, 2023

neheb commented Oct 17, 2023

neheb commented Oct 18, 2023

neheb commented Oct 18, 2023

Youw Oct 19, 2023

Choose a reason for hiding this comment

Youw Nov 21, 2023

Choose a reason for hiding this comment

neheb Nov 21, 2023

Choose a reason for hiding this comment

tormodvolden commented Oct 19, 2023

neheb commented Oct 19, 2023 • edited

tormodvolden commented Nov 5, 2023

neheb commented Nov 5, 2023

neheb commented Nov 21, 2023

Youw commented Nov 21, 2023

tormodvolden commented Nov 27, 2023

neheb commented Nov 27, 2023

hjelmn commented Nov 28, 2023

hjelmn commented Nov 28, 2023

Youw commented Oct 16, 2023 •

edited

mcuee commented Oct 16, 2023 •

edited

tormodvolden commented Oct 17, 2023 •

edited

tormodvolden commented Oct 17, 2023 •

edited

neheb commented Oct 19, 2023 •

edited