Skip to content

vcgencmd: Avoid compiler complaints#65

Open
popcornmix wants to merge 1 commit into
raspberrypi:masterfrom
popcornmix:compiler_warn
Open

vcgencmd: Avoid compiler complaints#65
popcornmix wants to merge 1 commit into
raspberrypi:masterfrom
popcornmix:compiler_warn

Conversation

@popcornmix

Copy link
Copy Markdown
Contributor

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

@popcornmix

Copy link
Copy Markdown
Contributor Author

@XECDesign does this fix your build issues?

@pelwell

pelwell commented Jan 15, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Contributor Author

Okay - dropped eeptools commit.

@XECDesign

Copy link
Copy Markdown
Contributor

Seems to build, yeah.

@XECDesign

Copy link
Copy Markdown
Contributor

I can update the package once this is merged.

@popcornmix

Copy link
Copy Markdown
Contributor Author

@pelwell is this okay?

@pelwell

pelwell commented Jan 16, 2024

Copy link
Copy Markdown
Collaborator

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

XECDesign commented Jan 16, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

chewi commented Feb 3, 2024

Copy link
Copy Markdown

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