Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Silence compiler warnings #1800

Merged
merged 2 commits into from
May 17, 2024

Conversation

stefanrueger
Copy link
Collaborator

Fixes #1799

The %hd converts the int argument to short before printing, so I suspect #1799 is a matter of overzealous compiler warning.

@MCUdude
Copy link
Collaborator

MCUdude commented Apr 28, 2024

Regarding compiler warnings, I'm getting the following one when building git main. No big deal, but I thought I'd mention it.

  LEX      lexer.c
/Library/Developer/CommandLineTools/usr/bin/make  all-recursive
Making all in .
  CC       avrdude-main.o
  CC       avrdude-whereami.o
  CC       avrdude-developer_opts.o
  CC       libavrdude_a-config_gram.o
  CC       libavrdude_a-lexer.o
lexer.c:2434:38: warning: comparison of integers of different signs: 'unsigned long' and 'int' [-Wsign-compare]
        if (((yy_n_chars) + number_to_move) > YY_CURRENT_BUFFER_LVALUE->yy_buf_size) {
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

@stefanrueger
Copy link
Collaborator Author

stefanrueger commented Apr 28, 2024

@MCUdude That's a known issue and we cannot do anything about it, as it's MacOS's flex that generates the code.

@@ -394,10 +394,10 @@ void dfu_show_info(struct dfu_dev *dfu)
msg_info(" USB Product : 0x%04hX\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also remove the h and (unsigned short) cast here?

Copy link
Collaborator Author

@stefanrueger stefanrueger Apr 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that would change what happens. Right now printf's %hx will (unsigned short) the promoted int argument that itself already has been (unsigned short)-ed. We can remove either the h or the (unsigned short) cast of the (assuming at most int-wide) argument and the print will be the same. If we remove both then something else will be printed if the arg doesn't fit in an unsigned short.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here an example:

#include <stdio.h>
int main() {
  int i = 0x78000;
  printf("%04hx\n", (unsigned short) i);
  printf("%04x\n", (unsigned short) i);
  printf("%04hx\n", i);
  printf("%04x\n", i);
}

@mcuee mcuee added the bug Something isn't working label Apr 29, 2024
@mcuee
Copy link
Collaborator

mcuee commented May 1, 2024

MinGW build still has some warnings.
https://github.com/avrdudes/avrdude/actions/runs/8870239966/job/24351804351

[ 68%] Building C object src/CMakeFiles/libavrdude.dir/updi_link.c.obj
In file included from D:/a/avrdude/avrdude/src/updi_link.c:36:
D:/a/avrdude/avrdude/src/updi_link.c: In function 'updi_physical_send':
D:/a/avrdude/avrdude/src/updi_link.c:93:14: warning: format '%lu' expects argument of type 'long unsigned int', but argument 8 has type 'size_t' {aka 'long long unsigned int'} [-Wformat=]
   93 |   pmsg_debug("sending %lu bytes [", len);
      |              ^~~~~~~~~~~~~~~~~~~~~  ~~~
      |                                     |
      |                                     size_t {aka long long unsigned int}
D:/a/avrdude/avrdude/src/avrdude.h:91:138: note: in definition of macro 'pmsg_debug'
   91 | #define pmsg_debug(...)     avrdude_message2(stderr, __LINE__, __FILE__, __func__, MSG2_PROGNAME|MSG2_FLUSH|MSG2_LEFT_MARGIN, MSG_DEBUG, __VA_ARGS__)
      |                                                                                                                                          ^~~~~~~~~~~
D:/a/avrdude/avrdude/src/updi_link.c:93:25: note: format string is defined here
   93 |   pmsg_debug("sending %lu bytes [", len);
      |                       ~~^
      |                         |
      |                         long unsigned int
      |                       %llu
D:/a/avrdude/avrdude/src/updi_link.c: In function 'updi_physical_recv':
D:/a/avrdude/avrdude/src/updi_link.c:117:14: warning: format '%lu' expects argument of type 'long unsigned int', but argument 8 has type 'size_t' {aka 'long long unsigned int'} [-Wformat=]
  117 |   pmsg_debug("received %lu bytes [", len);
      |              ^~~~~~~~~~~~~~~~~~~~~~  ~~~
      |                                      |
      |                                      size_t {aka long long unsigned int}
D:/a/avrdude/avrdude/src/avrdude.h:[91](https://github.com/avrdudes/avrdude/actions/runs/8870239966/job/24351804351#step:5:92):138: note: in definition of macro 'pmsg_debug'
   91 | #define pmsg_debug(...)     avrdude_message2(stderr, __LINE__, __FILE__, __func__, MSG2_PROGNAME|MSG2_FLUSH|MSG2_LEFT_MARGIN, MSG_DEBUG, __VA_ARGS__)
      |                                                                                                                                          ^~~~~~~~~~~
D:/a/avrdude/avrdude/src/updi_link.c:117:26: note: format string is defined here
  117 |   pmsg_debug("received %lu bytes [", len);
      |                        ~~^
      |                          |
      |                          long unsigned int
      |                        %llu

@dl8dtl
Copy link
Contributor

dl8dtl commented May 1, 2024

size_t {aka long long unsigned int}

size_t requires format letter z

@stefanrueger
Copy link
Collaborator Author

size_t requires format letter z

Correct; this was introduced with C99, but we have in the past tried to be even compatible with pre-1999 C-libraries (#1608) so probably better to cast len to (unsigned long) in this case. Just pushed a commit for this

@dl8dtl
Copy link
Contributor

dl8dtl commented May 3, 2024

I wouldn't do so unless MSVC is still that far behind to require it.
We're requiring a lot of C99 already elsewhere anyway.

@stefanrueger
Copy link
Collaborator Author

The C99 discussion was quite recent. Full details at #1594. I don't recall the details, but think someone built AVRDUDE in Windows 7 (that couldn't be upgraded) and the only problem they faced was %z[idux] conversion. Apparently, the rest of the code was fine (at least what they needed) and it isn't much effort to do without %z[idux].

@dl8dtl
Copy link
Contributor

dl8dtl commented May 3, 2024

Quite sad, it's only 15 years that have gone since …

@mcuee
Copy link
Collaborator

mcuee commented May 3, 2024

@stefanrueger and @dl8dtl

My suggestion is to give up on that paticular configuration -- mingw32 cross compiler toolchain under Windows 7.

If you read my comments in #1594, there is no issue using MSVC build and normal MSYS2 mingw32/64 build.

@mcuee
Copy link
Collaborator

mcuee commented May 3, 2024

I wouldn't do so unless MSVC is still that far behind to require it. We're requiring a lot of C99 already elsewhere anyway.

MSVC is not the problem here. So one less worry.

@mcuee
Copy link
Collaborator

mcuee commented May 3, 2024

@dl8dtl and @stefanrueger and @MCUdude

Quite sad, it's only 15 years that have gone since …

Let's face it, Windows 7 was released on 22-Sept-2009 and Microsoft stopped the extended support on 10-Jan-2023 (main line support ended on 14-Jan-2020). I think it is perfectly fine that we drop the support of Windows 7 altogether.

I do not have Windows 7 machine and I am not so sure if anyone of use still wants to carry out test under Windows 7.

Same for Windows 8/8.1 -- both reached EoL status as well.
https://learn.microsoft.com/en-us/lifecycle/faq/windows
https://support.microsoft.com/en-us/windows/windows-8-1-support-ended-on-january-10-2023-3cfd4cde-f611-496a-8057-923fba401e93

Windows 8.1 reached the end of Mainstream Support on January 9, 2018, and reached the end of Extended Support on January 10, 2023.

@dl8dtl
Copy link
Contributor

dl8dtl commented May 3, 2024

You're right about Win7. But then, I think it's fine to use the 'z' modifier, rather than casting stuff around.

@mcuee
Copy link
Collaborator

mcuee commented May 3, 2024

You're right about Win7. But then, I think it's fine to use the 'z' modifier, rather than casting stuff around.

No issue with that. I just want to establish that we will not officially support Windows 7/8/8.1 from now on. It may still work well. However, if there are issues, we may rely on the users to provide non-intrusive patches if they really care about Windows 7/8/8.1.

BTW, MSYS2 has dropped 32bit Windows support of libusb/libusb-compat/libftdi, so we have already dropped 32bit mingw support of avrdude as well. Take note Windows 10/11 only supports x86_64 and not 32bit x86.

@mcuee
Copy link
Collaborator

mcuee commented May 3, 2024

@stefanrueger and @dl8dtl

Not sure if we can fix MSYS2 clang64 compiler warnings as well.
https://github.com/avrdudes/avrdude/actions/runs/8935908735/job/24545265227

D:/a/avrdude/avrdude/src/ser_win32.c:436:3: warning: comparison of integers of different signs: 'SOCKET' (aka 'unsigned long long') and 'const int' [-Wsign-compare]
  436 |                 FD_SET(fd->ifd, &rfds);
      |                 ^~~~~~~~~~~~~~~~~~~~~~
D:/a/_temp/msys64/clang64/include/psdk_inc/_fd_types.h:77:40: note: expanded from macro 'FD_SET'
   77 |                 if (((fd_set *)(set))->fd_array[__i] == (fd)) {         \
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^   ~~
D:/a/avrdude/avrdude/src/ser_win32.c:[55](https://github.com/avrdudes/avrdude/actions/runs/8935908735/job/24545265227#step:5:56)7:3: warning: comparison of integers of different signs: 'SOCKET' (aka 'unsigned long long') and 'const int' [-Wsign-compare]
  557 |                 FD_SET(fd->ifd, &rfds);
      |                 ^~~~~~~~~~~~~~~~~~~~~~
D:/a/_temp/msys64/clang64/include/psdk_inc/_fd_types.h:77:40: note: expanded from macro 'FD_SET'
   77 |                 if (((fd_set *)(set))->fd_array[__i] == (fd)) {         \
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^   ~~
2 warnings generated.

@stefanrueger
Copy link
Collaborator Author

Not sure if we can fix MSYS2 clang64 compiler warnings as well.

See here

@stefanrueger stefanrueger merged commit dc845ec into avrdudes:main May 17, 2024
13 checks passed
@stefanrueger stefanrueger deleted the silence-printf-warnings branch May 17, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

format mismatch warnings
4 participants