Disable -Wmissing-prototypes for internal linkage functions that aren't explicitly marked "static"""

[dblaikie]

Summary

Some functions can end up non-externally visible despite not being
declared "static" or in an unnamed namespace in C++ - such as by having
parameters that are of non-external types.

Such functions aren't mistakenly intended to be defining some function
that needs a declaration. They could be maybe more legible (except for
the operator new example) with an explicit static, but that's a
stylistic thing outside what should be addressed by a warning.

This reapplies 275c56226d7fbd6a4d554807374f78d323aa0c1c - once we figure
out what to do about the change in behavior for -Wnon-c-typedef-for-linkage
(this reverts the revert commit 85ee1d3ca1d06b6bd3477515b8d0c72c8df7c069)

[manojgupta]

For the background, we had hit this in Chrome OS when building bluetooth code.

This is the one of structs hitting the issue where the warning got promoted to an error:

typedef struct {
<snip>
private:

static std::string AppendCapability(std::string& result, bool append,
                                    const std::string& name) {
 <snip>
}

} btav_a2dp_codec_config_t;

Previously this was a warning that was suppressed in this code upstream using -Wno-non-c-typedef-for-linkage but now it turns into a non-suppressible error.

The exact code actually comes from aosp, but this is the Chrome OS public accessible copy:
https://chromium.googlesource.com/aosp/platform/packages/modules/Bluetooth/+/main/system/include/hardware/bt_av.h

[dblaikie]

Ping

[aaron.ballman]

once we figure out what to do about the change in behavior for -Wnon-c-typedef-for-linkage

The devil is in the details; I'm not sure what to do here. I don't think there's a way to compute the visibility without also computing the linkage, so I don't think there's a way to get the old behavior in the Chrome OS case as well as suppressing the diagnostic in the case you want to cover. I'm sympathetic to the issue, but this may be a case where it's a matter of picking which behavior is more important because we may not be able to have both (at least, not without work to tease apart linkage and visibility if that's possible).

[dblaikie]

In D121328#3396984, @aaron.ballman wrote:

once we figure out what to do about the change in behavior for -Wnon-c-typedef-for-linkage

The devil is in the details; I'm not sure what to do here. I don't think there's a way to compute the visibility without also computing the linkage, so I don't think there's a way to get the old behavior in the Chrome OS case as well as suppressing the diagnostic in the case you want to cover. I'm sympathetic to the issue, but this may be a case where it's a matter of picking which behavior is more important because we may not be able to have both (at least, not without work to tease apart linkage and visibility if that's possible).

Ah, fair. To /me/ it doesn't feel like typedef struct { void f() { } } x; is something we /really/ need to support, but clearly some code does use this, but it should be relatively cheap/simple to migrate away from that idiom and make the code more C++ idiomatic/simpler. But maybe there's some huge legacy of code using this idiom, such that breaking it would be costly. Don't really know.

@rsmith maybe you've got an idea of whether this (C-style typedef/name-for-linkage purposes with structs with member functions) is worth supporting?

[jyknight]

Given that this is an error error: functions cannot be declared in an anonymous struct (thus you can assume that function declarations inside a struct imply that the struct MUST have a linkage name)

struct {
  static int foo();
};

then I don't think there's any conceptual reason why we cannot emit the desired diagnostics without the undesired side-effect.

[deansturtevant]

Note that some fix is important to make if we think that the -Wmissing-prototypes warning is valuable, because there are cases where it currently would fire where the function cannot explicitly be given internal linkage, e.g.
namespace {
struct Initialized {};
} // namespace
void* operator new(size_t size, const Initialized&) {

void* p = malloc(size);
memset(p, 0, size);
return p;

}
operator new is only allowed to be defined in the global namespace.
Currently this will cause the warning to be emitted, even though there is no way to declare it outside the TU.

jyknight clarified that what he was proposing was that instead of calling FD->isExternallyVisible() as currently written, call a function which does the same thing except that it doesn't check whether the struct has a visible name (because that can be assumed here).
This function *could* be isExternallyVisible with a new boolean argument (WDYT?)
Another approach would be to move the call to ShouldWarnAboutMissingPrototypes() to a different point in the process, where calling isExternallyVisible() is *not* problematic. WDYT?

[deansturtevant]

Aha! (I think).
If the code to test "isExternalVisible" is executed *after* the code to test whether it's a C++ member function (the very next test), then the problem wouldn't arise.

[dblaikie]

Try Dean's suggestion of moving this check further down

[deansturtevant]

Thanks David. Is it possible to add a regression test for the problem that cropped up with the first try?

[dblaikie]

Add regression test for the -Wnon-c-typedef-for-linkage issue

[dblaikie]

In D121328#3405623, @deansturtevant wrote:

Aha! (I think).
If the code to test "isExternalVisible" is executed *after* the code to test whether it's a C++ member function (the very next test), then the problem wouldn't arise.

Oh, fair - good call. Updated with that change & it does address the warning issue. Added a regression test for that too (not sure if there are other cases worth covering too... nothing that springs to mind.

[deansturtevant]

This looks good from my standpoint.

[dblaikie]

@aaron.ballman wouldn't mind your take on this to see if this seems sufficiently robust, tested, etc. (should I move the isExternallyVisible check even further down? So its side effects are even less impactful (maybe there are other warnings that care about this sort of thing?) )

[deansturtevant]

I had given this some thought. I have a very limited knowledge of the code
in this space. IIUC this only matters if the idea of linkage can change,
which would happen only if a tag name needs manufacturing, which can only
happen in the case we ran across (someone check my logic here?). The other
thing that factored into my thinking is that this seemed to be the only
reported downside. I don't feel qualified to make a decision whether moving
the code farther down would be better.

[aaron.ballman]

In D121328#3406017, @dblaikie wrote:

@aaron.ballman wouldn't mind your take on this to see if this seems sufficiently robust, tested, etc. (should I move the isExternallyVisible check even further down? So its side effects are even less impactful (maybe there are other warnings that care about this sort of thing?) )

Oh this turned out to be a MUCH easier solution than I was thinking, good catch! The changes LGTM as-is. I think we could potentially move this check further down given that we know it has side effects. I *think* it'd be fine directly above the for loop, at least from looking at the implementations of those other functions, I don't see others that are caching bits in the way that isExternallyVisible() ends up doing. However, I don't insist on moving it, either.

clang/lib/Sema/SemaDecl.cpp

ShouldWarnAboutMissingPrototype(const FunctionDecl *FD,
   // Don't warn for OpenCL kernels.
   if (FD->hasAttr<OpenCLKernelAttr>())
     return false;
 
   // Don't warn on explicitly deleted functions.
   if (FD->isDeleted())
     return false;
 
+  // Don't warn on implicitly local functions (such as having local-typed
+  // parameters).
+  if (!FD->isExternallyVisible())
+    return false;
+
   for (const FunctionDecl *Prev = FD->getPreviousDecl();
        Prev; Prev = Prev->getPreviousDecl()) {
     // Ignore any declarations that occur in function or method
     // scope, because they aren't visible from the header.
     if (Prev->getLexicalDeclContext()->isFunctionOrMethod())
       continue;
 
     PossiblePrototype = Prev;

clang/test/SemaCXX/anonymous-struct.cpp

 // Ensure we don't compute the linkage of a member function just because it
 // happens to have the same name as a builtin.
 namespace BuiltinName {
   // Note that this is not an error: we didn't trigger linkage computation in this example.
   typedef struct { // expected-warning {{anonymous non-C-compatible type}}
     void memcpy(); // expected-note {{due to this member}}
   } A; // expected-note {{given name 'A' for linkage purposes by this typedef}}
 }
+namespace inline_defined_static_member {
+typedef struct { // expected-warning {{anonymous non-C-compatible type}}
+  static void f() { // expected-note {{due to this member}}
+  }
+} A; // expected-note {{given name 'A' for linkage purposes by this typedef}}
+}

clang/test/SemaCXX/warn-missing-prototypes.cpp

 };
 
 // Don't warn on explicitly deleted functions.
 void j() = delete;
 
 extern void k() {} // expected-warning {{no previous prototype for function 'k'}}
 // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
 // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{.*}}-[[@LINE-2]]:{{.*}}}:"{{.*}}"
+
+namespace {
+struct anon { };
+}
+
+// No warning because this has internal linkage despite not being declared
+// explicitly 'static', owing to the internal linkage parameter.
+void l(anon) {
+}
+
+void *operator new(decltype(sizeof(3)) size, const anon &) throw() {
+  return nullptr;
+}