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.

Sign up for GitHub

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

Jump to bottom

Maybe add -Wformat-type-confusion to the build as much as we can #7935

Closed
bzbarsky-apple opened this issue Jun 26, 2021 · 2 comments · Fixed by #7993
Closed

Maybe add -Wformat-type-confusion to the build as much as we can #7935

bzbarsky-apple opened this issue Jun 26, 2021 · 2 comments · Fixed by #7993

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

We are not getting all possible format confusion warnings right now.

Proposed Solution

See #7921 (comment) -- add -Wformat-type-confusion to catch more.

I tried adding it in build/config/compiler/BUILD.gn but then I get errors in third-party code like so:

../../third_party/lwip/repo/lwip/src/core/ipv6/ip6.c:1173:21: error: format specifies type 'unsigned short' but the argument has type 'u8_t' (aka 'unsigned char') [-Werror,-Wformat-type-confusion]
                    IP6H_NEXTH(ip6hdr),
                    ^~~~~~~~~~~~~~~~~~~

@mspang is there somewhere we can add this that will get used for our code but not third-party code?
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 26, 2021
Merged
@bzbarsky-apple bzbarsky-apple changed the title Maybe -Wformat-type-confusion to the build as much as we can Maybe add -Wformat-type-confusion to the build as much as we can Jun 26, 2021
mspang added a commit to mspang/connectedhomeip that referenced this issue Jun 28, 2021
@mspang
RFC: Add common 3rd party warnings config target
521c81f 
There is an idiom for relaxing warnings for third party in the GN:

  configs -= [ "//buildsys/1st_party_warnings" ]
  configs += [ "//buildsys/3rd_party_warnings" ]

The trouble is that "//buildsys/1st_party_warnings" is an object from
the core build system that we have to name which introduces some
coupling (e.g., projects may have the same pattern with different target
names and this would not work). To minimize this coupling, indirect this
through the build_overrides directory where the top level project can
provide suitable targets (or not, if the defaults already work).

This is provided for discussion in issue project-chip#7935
mspang added a commit to mspang/connectedhomeip that referenced this issue Jun 28, 2021
@mspang
RFC: Add common 3rd party warnings config target
4655503 
There is an idiom for relaxing warnings for third party in the GN:

  configs -= [ "//buildsys/1st_party_warnings" ]
  configs += [ "//buildsys/3rd_party_warnings" ]

The trouble is that "//buildsys/1st_party_warnings" is an object from
the core build system that we have to name which introduces some
coupling (e.g., projects may have the same pattern with different target
names and this would not work). To minimize this coupling, indirect this
through the build_overrides directory where the top level project can
provide suitable targets (or not, if the defaults already work).

This is provided for discussion in issue project-chip#7935
@mspang mspang mentioned this issue Jun 28, 2021
Merged
@mspang
Copy link
Contributor

mspang commented Jun 28, 2021

We actually already are disabling warnings in lwip

  config("${lwip_target_name}_warnings") {
    cflags = [
      "-Wno-address",
      "-Wno-format",
      "-Wno-type-limits",
      "-Wno-unused-variable",
      "-Wno-maybe-uninitialized",
    ]
  }

We can split the warnings_default config into warnings_default and warnings_third_party if we want but it's a little bit tricky to do it without introducing coupling with the name & organization of the core build rules (which I'm really trying to minimize, so that you can drop in CHIP into a project with different versions of them which is something that we are doing).

One way we could do it is #7959

mspang added a commit to mspang/connectedhomeip that referenced this issue Jun 28, 2021
@mspang
RFC: Add common 3rd party warnings config target
c7eea5c 
There is an idiom for relaxing warnings for third party in GN:

  configs -= [ "//buildsys/1st_party_warnings" ]
  configs += [ "//buildsys/3rd_party_warnings" ]

The trouble is that "//buildsys/1st_party_warnings" is an object from
the core build system that we have to name which introduces some
coupling (e.g., projects may have the same pattern with different target
names and this would not work). To minimize this coupling, indirect this
through the build_overrides directory where the top level project can
provide suitable targets (or not, if the defaults already work).

This is provided for discussion in issue project-chip#7935
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 29, 2021
Closed
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 29, 2021
@bzbarsky-apple
Add -Wformat-type-confusion to catch a wider range of format string e…
5a14540 
…rrors.

This is supported by clang, not gcc, hence adding this only to the
clang cflags in the strict warnings config.

lwip triggers this warning a lot, so we disable it there.

Fixes project-chip#7935
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 29, 2021
Merged
@bzbarsky-apple
Copy link
Contributor Author

We actually already are disabling warnings in lwip

Perfect, thank you!

woody-apple pushed a commit that referenced this issue Jun 29, 2021
@bzbarsky-apple
Add -Wformat-type-confusion to catch a wider range of format string e…
33fa170 
…rrors. (#7993)

This is supported by clang, not gcc, hence adding this only to the
clang cflags in the strict warnings config.

lwip triggers this warning a lot, so we disable it there.

Fixes #7935
mspang added a commit to mspang/connectedhomeip that referenced this issue Jul 5, 2021
@mspang
RFC: Add common 3rd party warnings config target
686b3f8 
There is an idiom for relaxing warnings for third party in GN:

  configs -= [ "//buildsys/1st_party_warnings" ]
  configs += [ "//buildsys/3rd_party_warnings" ]

The trouble is that "//buildsys/1st_party_warnings" is an object from
the core build system that we have to name which introduces some
coupling (e.g., projects may have the same pattern with different target
names and this would not work). To minimize this coupling, indirect this
through the build_overrides directory where the top level project can
provide suitable targets (or not, if the defaults already work).

This is provided for discussion in issue project-chip#7935
bzbarsky-apple pushed a commit that referenced this issue Jul 10, 2021
@mspang
RFC: Add common 3rd party warnings config target (#7959)
97ab77f 
There is an idiom for relaxing warnings for third party in GN:

  configs -= [ "//buildsys/1st_party_warnings" ]
  configs += [ "//buildsys/3rd_party_warnings" ]

The trouble is that "//buildsys/1st_party_warnings" is an object from
the core build system that we have to name which introduces some
coupling (e.g., projects may have the same pattern with different target
names and this would not work). To minimize this coupling, indirect this
through the build_overrides directory where the top level project can
provide suitable targets (or not, if the defaults already work).

This is provided for discussion in issue #7935
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this issue Sep 23, 2021
@bzbarsky-apple
Add -Wformat-type-confusion to catch a wider range of format string e…
1c62898 
…rrors. (project-chip#7993)

This is supported by clang, not gcc, hence adding this only to the
clang cflags in the strict warnings config.

lwip triggers this warning a lot, so we disable it there.

Fixes project-chip#7935
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this issue Sep 23, 2021
@mspang
RFC: Add common 3rd party warnings config target (project-chip#7959)
263c392 
There is an idiom for relaxing warnings for third party in GN:

  configs -= [ "//buildsys/1st_party_warnings" ]
  configs += [ "//buildsys/3rd_party_warnings" ]

The trouble is that "//buildsys/1st_party_warnings" is an object from
the core build system that we have to name which introduces some
coupling (e.g., projects may have the same pattern with different target
names and this would not work). To minimize this coupling, indirect this
through the build_overrides directory where the top level project can
provide suitable targets (or not, if the defaults already work).

This is provided for discussion in issue project-chip#7935
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mspang @bzbarsky-apple