This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [builtin] Add public functions prototypes
AbandonedPublic

Authored by krytarowski on May 13 2020, 3:28 AM.

Details

Reviewers
None
Summary

Suppress compiler errors with raised warning levels.

Diff Detail

Event Timeline

krytarowski created this revision.May 13 2020, 3:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 3:28 AM
Herald added subscribers: Restricted Project, jfb, dberris. · View Herald Transcript

I'm guessing that this is explicitly enabling the missing prototypes warning. I'm not a huge fan of that for compilation units that don't have public headers, it makes the code more complex and doesn't catch any bugs. It's valuable only when you have a public header and want to ensure that it's in sync with the implementation.

I'm guessing that this is explicitly enabling the missing prototypes warning. I'm not a huge fan of that for compilation units that don't have public headers, it makes the code more complex and doesn't catch any bugs. It's valuable only when you have a public header and want to ensure that it's in sync with the implementation.

OK. Let's call it appeasing of a compiler. I could add a header atomic.h if wanted, but in this case it is better to ship the prototypes inline. I prefer to get this buildable out of the box without building it each time with -Wno*.

It should already be buildable out of the box. The missing prototypes warning is opt-in. I am not particularly in favour of changes motivated solely by an external build system that explicitly opts in to warnings that don't make sense for a particular compilation unit.

krytarowski added a comment.EditedMay 13 2020, 5:47 AM

It should already be buildable out of the box. The missing prototypes warning is opt-in. I am not particularly in favour of changes motivated solely by an external build system that explicitly opts in to warnings that don't make sense for a particular compilation unit.

There is prior art and practice.

We support building of compiler-rt parts with GCC and address build errors. Why to keep atomic.c an exception (other than making it harder to reuse)?

The overhead of function having declarations is very low.

krytarowski added a comment.EditedMay 13 2020, 5:50 AM

Example error:

atomic.c: At top level:
atomic.c:168:6: error: no previous prototype for ‘__atomic_load_c’ [-Werror=missing-prototypes]
 void __atomic_load_c(int size, void *src, void *dest, int model) {
      ^~~~~~~~~~~~~~~

A notable example from compiler-rt are sanitizers. Almost the entire code is as-is taken into GCC as libsanitizer, with included external build rules and compiled with GCC.

jfb added a comment.May 13 2020, 1:50 PM

We can't enforce compiler diagnostics unless they're added to the CMake build. Which files are now expected to build with this diagnostic (and others?). This specific diagnostic adds zero value, so even if the cost is small, the upside is still zero.

@krytarowski , as I read this change you are:

  • Building with an external build system.
  • Enabling a warning that is not part of the default set and adds no value to a compilation unit of this nature.
  • Explicitly making that warning an error (which it is not by default).
  • Noticing that the file then does not compile.
  • Proposing a patch that adds complexity to the code that everyone else has to maintain, to fix this specific use case.

If I am misunderstanding the flow, please explain it to me. You are stating that this is about making the file compile out-of-the-box with gcc, but when I compile with gcc 8, I don't see that warning, so this is not about compiling with GCC, it is about compiling with a specific external build system. Generally, I would consider that the maintenance burden of using an external build system should fall on the maintainers of that build system, not the upstream project. FreeBSD, for example, builds clang, lldb, compiler-rt, and libc++ as part of the FreeBSD buildworld bmake-based build system, which defaults to -Wmissing-prototypes, but turns off this warning for LLVM (FreeBSD compiles most of compiler-rt into a binary called libgcc_s.so, for compatibility with gcc).

The -Wmissing-prototypes warning is useful for catching the following errors:

  • Functions that are exposed to the library's public symbols but not the API in the headers.
  • Functions that were meant to be declared static and weren't.
  • Accidental mismatches between the implementation and header file.

None of these apply to this file because the API contract is an ABI contract that is not exposed via a header. Neither clang nor gcc loads headers for functions provided by the runtime library (it might be nice if they did) and so even if we had a header and we caught mismatches between it and the implementation here, we wouldn't catch mismatches between the header and the calls inserted by the compiler. It would be nice to teach clang to spit out a header for all of the library calls that it knows about and then #include that in this file and enable the warning, but enabling the warning to catch a mismatch between a line of code and a copied-and-pasted version of the same code one line earlier adds complexity for zero benefit.

krytarowski abandoned this revision.May 14 2020, 6:18 AM

OK, we will need to live with -Wno*.

OK, we will need to live with -Wno*.

You don't need -Wno-missing-prototypes, you just need to not explicitly add -Wmissing-prototypes. The missing prototypes warning is opt-in. Just don't opt in. If you compile this file with GCC without specifying any warning flags, you do not see these warnings.