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

vcgencmd: Avoid compiler complaints #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

popcornmix
Copy link
Contributor

Latest compilers are more fussy and this doesn't build with debian build scripts. Fix this.

@popcornmix
Copy link
Contributor Author

@XECDesign does this fix your build issues?

@pelwell
Copy link
Collaborator

pelwell commented Jan 15, 2024

See #64.

In file included from /usr/include/string.h:535,
                 from /<<PKGBUILDDIR>>/vcgencmd/vcgencmd.c:34:
In function ‘strncat’,
    inlined from ‘gencmd’ at /<<PKGBUILDDIR>>/vcgencmd/vcgencmd.c:109:4,
    inlined from ‘main’ at /<<PKGBUILDDIR>>/vcgencmd/vcgencmd.c:152:14:
/usr/include/arm-linux-gnueabihf/bits/string_fortified.h:138:10: warning: ‘__builtin___strncat_chk’ specified bound 1024 equals destination size [-Wstringop-overflow=]
  138 |   return __builtin___strncat_chk (__dest, __src, __len,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  139 |                                   __glibc_objsize (__dest));
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~

can be seen with "gcc -O2 -Wall"
@popcornmix popcornmix changed the title vcgencmd/eeptools: Avoid compiler complaints vcgencmd: Avoid compiler complaints Jan 15, 2024
@popcornmix
Copy link
Contributor Author

Okay - dropped eeptools commit.

@XECDesign
Copy link
Contributor

Seems to build, yeah.

@XECDesign
Copy link
Contributor

I can update the package once this is merged.

@popcornmix
Copy link
Contributor Author

@pelwell is this okay?

@pelwell
Copy link
Collaborator

pelwell commented Jan 16, 2024

I don't understand (without seeing the errors) why memset (which will happily copy beyond the end of the string) is somehow more acceptable than strncpy, but I'm happy for this to be merged.

@XECDesign
Copy link
Contributor

XECDesign commented Jan 16, 2024

Full disclosure: I didn't try building from head of tree without this commit. So if there's a chance it's not needed, I can try without it.

Update: seems happy without it.

@XECDesign
Copy link
Contributor

I was curious why it wasn't failing and it looks like some cmake files have this line: set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wextra -Werror -pedantic")

vcgencmd doesn't, so it only inherits the CFLAGS passed down by Debian's build system - CFLAGS=-g -O2 -ffile-prefix-map=<snip>=. -fstack-protector-strong -Wformat -Werror=format-security

@popcornmix
Copy link
Contributor Author

I don't understand (without seeing the errors) why memset (which will happily copy beyond the end of the string) is somehow more acceptable than strncpy, but I'm happy for this to be merged.

In the commit message was the error:

In file included from /usr/include/string.h:535,
from /<>/vcgencmd/vcgencmd.c:34:
In function ‘strncat’,
inlined from ‘gencmd’ at /<>/vcgencmd/vcgencmd.c:109:4,
inlined from ‘main’ at /<>/vcgencmd/vcgencmd.c:152:14:
/usr/include/arm-linux-gnueabihf/bits/string_fortified.h:138:10: warning: ‘__builtin___strncat_chk’ specified bound 1024 equals destination size [-Wstringop-overflow=]
138 | return __builtin___strncat_chk (__dest, __src, __len,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
139 | __glibc_objsize (__dest));
| ~~~~~~~~~~~~~~~~~~~~~~~~~

it seems strncat triggers some additional bounds checking that memcpy does not.

@chewi
Copy link

chewi commented Feb 3, 2024

Please can we not include -Werror in the default flags as it's a pain for both distributions and users building themselves. Perhaps we could only add it when CMAKE_BUILD_TYPE is Debug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants