Add `-Wmissing-prototypes` and fix issues it finds. #4019

chandlerc · 2024-06-02T04:36:38Z

Most of these are places where we failed to include a header file and
simply never got an error about this. The fix is to include the header
file.

Most other cases are functions that should have been marked static but
were not. Finding all of these was a main motivation for me enabling the
warning despite how much work it is.

One complicating factor was that we weren't including the handle.h for
all the state-based handler functions. While this isn't a tiny amount of
code, it is just declarations and doesn't add any extra dependencies. It
also lets us have the checking for which functions need to be static
and which don't. For the parse library I had to add the handle.h
header as well, I tried to match the design of it in check.

I have also had to work around a bug in the warning, but given the value
it seems to be providing, that seems reasonable. I've filed the bug
upstream: llvm/llvm-project#94138

I also had to use some hacks to work around limitations of Bazel rules
that wrap cc_library rules and don't expose copts. I filed a bug for
cc_proto_library specifically:
~~bazelbuild/bazel#22610~~
bazelbuild/bazel#4446

zygoloid · 2024-06-02T17:31:41Z

toolchain/parse/handle.h

+namespace Carbon::Parse {
+
+// Declare handlers for each parse state.
+#define CARBON_PARSE_STATE(Name) auto Handle##Name(Context& context) -> void;


@jonmeow I think this is worth discussion to make sure we're all on the same page:

Previously, we defined these functions in the relevant handle_*.cpp files with no forward declaration, which I think was probably intentional. Adding the forward declarations means we get the substantial advantage of being able to detect missing static, but also means the Handle* functions are all in scope in all handle_*.cpp files, whereas we probably don't want them to be able to invoke each other.

Similarly in check/, where we had the header file already but didn't include it from the handle_*.cpp files.

I think this change is a good thing nonetheless. (It would also catch issues where the return type of a Handle* function is wrong.)

Now that you remind, me I remember this being intentional. Sorry I forgot about that.

But it is definitely something to discuss -- not at all sure what the exact balance of tradeoffs is here. I was honestly surprised how many real issues -Wmissing-prototypes found (completely outside of any handle.h stuff) where we forgot headers and didn't use static. So mostly sending this to see if there is a good pattern we could follow here. Not at all attached to specifically having all the Handle* functions in one header though, very happy if we want some different pattern or way of approaching this.

I think there's probably more benefit to prototype matching than avoiding the risk of recursive calls this way. I'd probably have made these functions on a class if I'd been allowed to, just to get the declaration match checking (it's a nuisance when a typo on names is only detected at link time, and I'm a little surprised this didn't seem to find any unused code where a state had been deleted but the handler left behind). Not needing to comment for static as much will be nice, too.

utils/treesitter/src/scanner.c

toolchain/parse/parse_fuzzer.cpp

jonmeow · 2024-06-03T15:13:04Z

toolchain/parse/handle.h

+namespace Carbon::Parse {
+
+// Declare handlers for each parse state.
+#define CARBON_PARSE_STATE(Name) auto Handle##Name(Context& context) -> void;


I think there's probably more benefit to prototype matching than avoiding the risk of recursive calls this way. I'd probably have made these functions on a class if I'd been allowed to, just to get the declaration match checking (it's a nuisance when a typo on names is only detected at link time, and I'm a little surprised this didn't seem to find any unused code where a state had been deleted but the handler left behind). Not needing to comment for static as much will be nice, too.

jonmeow · 2024-06-03T15:14:54Z

explorer/fuzzing/BUILD

@@ -110,6 +110,7 @@ cc_fuzz_test(
 name = "explorer_fuzzer",
 size = "small",
 srcs = ["explorer_fuzzer.cpp"],
+ copts = ["-Wno-missing-prototypes"],


Why this approach here, versus what's done for other fuzzers?

Because the explorer uses macros and such to instantiate the proto-based fuzzer. I didn't want to guess at what the right or wrong declarations would be, so I just turned off the warning. It also seemed more in line with disabling clang-tidy before spending more time trying to figure out a principled solution.

Thanks for explaining, would it be worth adding a small comment to this effect?

Adding comment.

Most of these are places where we failed to include a header file and simply never got an error about this. The fix is to include the header file. Most other cases are functions that should have been marked `static` but were not. Finding all of these was a main motivation for me enabling the warning despite how much work it is. One complicating factor was that we weren't including the `handle.h` for all the state-based handler functions. While this isn't a tiny amount of code, it is just declarations and doesn't add any extra dependencies. It also lets us have the checking for which functions need to be `static` and which don't. For the `parse` library I had to add the `handle.h` header as well, I tried to match the design of it in `check`. I have also had to work around a bug in the warning, but given the value it seems to be providing, that seems reasonable. I've filed the bug upstream: llvm/llvm-project#94138 I also had to use some hacks to work around limitations of Bazel rules that wrap `cc_library` rules and don't expose `copts`. I filed a bug for `cc_proto_library` specifically: bazelbuild/bazel#22610

chandlerc · 2024-06-04T09:10:57Z

PTAL and lemme know if this is good, or a different approach. Happy to also look into class methods or something else if more appropriate.

jonmeow

Note there's still a build failure, maybe the changes caused a clang-tidy issue to be flagged?

But feel free to merge once that's fixed.

jonmeow · 2024-06-04T16:59:00Z

explorer/fuzzing/BUILD

@@ -110,6 +110,7 @@ cc_fuzz_test(
 name = "explorer_fuzzer",
 size = "small",
 srcs = ["explorer_fuzzer.cpp"],
+ copts = ["-Wno-missing-prototypes"],


Thanks for explaining, would it be worth adding a small comment to this effect?

github-actions bot added explorer Action items related to Carbon explorer code toolchain labels Jun 2, 2024

github-actions bot requested review from jonmeow and zygoloid June 2, 2024 04:36

zygoloid reviewed Jun 2, 2024

View reviewed changes

jonmeow approved these changes Jun 3, 2024

View reviewed changes

chandlerc force-pushed the missing-prototypes branch from 87b266b to 9f363af Compare June 4, 2024 09:06

chandlerc force-pushed the missing-prototypes branch from 9f363af to 8375962 Compare June 4, 2024 09:10

chandlerc requested review from jonmeow and zygoloid June 4, 2024 09:11

fix clang-tidy

42c9a8e

jonmeow approved these changes Jun 4, 2024

View reviewed changes

add comment

a012a78

chandlerc enabled auto-merge June 4, 2024 18:09

chandlerc added this pull request to the merge queue Jun 4, 2024

Merged via the queue into carbon-language:trunk with commit 8c64f0b Jun 4, 2024
7 checks passed

chandlerc deleted the missing-prototypes branch June 4, 2024 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `-Wmissing-prototypes` and fix issues it finds. #4019

Add `-Wmissing-prototypes` and fix issues it finds. #4019

chandlerc commented Jun 2, 2024 •

edited

Loading

zygoloid Jun 2, 2024

chandlerc Jun 2, 2024

jonmeow Jun 3, 2024

jonmeow Jun 3, 2024

jonmeow Jun 3, 2024

chandlerc Jun 4, 2024

jonmeow Jun 4, 2024

chandlerc Jun 4, 2024

chandlerc commented Jun 4, 2024

jonmeow left a comment •

edited

Loading

jonmeow Jun 4, 2024

Add -Wmissing-prototypes and fix issues it finds. #4019

Add -Wmissing-prototypes and fix issues it finds. #4019

Conversation

chandlerc commented Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc commented Jun 4, 2024

jonmeow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `-Wmissing-prototypes` and fix issues it finds. #4019

Add `-Wmissing-prototypes` and fix issues it finds. #4019

chandlerc commented Jun 2, 2024 •

edited

Loading

jonmeow left a comment •

edited

Loading