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

[krytarowski]

Summary

Suppress compiler errors with raised warning levels.

[theraven]

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.

[krytarowski]

In D79846#2033589, @theraven wrote:

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*.

[theraven]

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]

In D79846#2033705, @theraven wrote:

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]

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]

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.

[theraven]

@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]

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

[theraven]

In D79846#2036260, @krytarowski wrote:

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.

compiler-rt/lib/builtins/atomic.c

case 16:                                                                   \
         LOCK_FREE_ACTION(__uint128_t);*/                                       \
       }                                                                        \
       break;                                                                   \
     }                                                                          \
   } while (0)
 
 /// An atomic load operation.  This is atomic with respect to the source
 /// pointer only.
+void __atomic_load_c(int size, void *src, void *dest, int model);
 void __atomic_load_c(int size, void *src, void *dest, int model) {
 #define LOCK_FREE_ACTION(type)                                                 \
   *((type *)dest) = __c11_atomic_load((_Atomic(type) *)src, model);            \
   return;
   LOCK_FREE_CASES();
 #undef LOCK_FREE_ACTION
   Lock *l = lock_for_pointer(src);
   lock(l);
   memcpy(dest, src, size);
   unlock(l);
 }
 
 /// An atomic store operation.  This is atomic with respect to the destination
 /// pointer only.
+void __atomic_store_c(int size, void *dest, void *src, int model);
 void __atomic_store_c(int size, void *dest, void *src, int model) {
 #define LOCK_FREE_ACTION(type)                                                 \
   __c11_atomic_store((_Atomic(type) *)dest, *(type *)src, model);              \
   return;
   LOCK_FREE_CASES();
 #undef LOCK_FREE_ACTION
   Lock *l = lock_for_pointer(dest);
   lock(l);
   memcpy(dest, src, size);
   unlock(l);
 }
 
 /// Atomic compare and exchange operation.  If the value at *ptr is identical
 /// to the value at *expected, then this copies value at *desired to *ptr.  If
 /// they  are not, then this stores the current value from *ptr in *expected.
 ///
 /// This function returns 1 if the exchange takes place or 0 if it fails.
 int __atomic_compare_exchange_c(int size, void *ptr, void *expected,
+                                void *desired, int success, int failure);
+int __atomic_compare_exchange_c(int size, void *ptr, void *expected,
                                 void *desired, int success, int failure) {
 #define LOCK_FREE_ACTION(type)                                                 \
   return __c11_atomic_compare_exchange_strong(                                 \
       (_Atomic(type) *)ptr, (type *)expected, *(type *)desired, success,       \
       failure)
   LOCK_FREE_CASES();
 #undef LOCK_FREE_ACTION
   Lock *l = lock_for_pointer(ptr);
   lock(l);
   if (memcmp(ptr, expected, size) == 0) {
     memcpy(ptr, desired, size);
     unlock(l);
     return 1;
   }
   memcpy(expected, ptr, size);
   unlock(l);
   return 0;
 }
 
 /// Performs an atomic exchange operation between two pointers.  This is atomic
 /// with respect to the target address.
+void __atomic_exchange_c(int size, void *ptr, void *val, void *old, int model);
 void __atomic_exchange_c(int size, void *ptr, void *val, void *old, int model) {
 #define LOCK_FREE_ACTION(type)                                                 \
   *(type *)old =                                                               \
       __c11_atomic_exchange((_Atomic(type) *)ptr, *(type *)val, model);        \
   return;
   LOCK_FREE_CASES();
 #undef LOCK_FREE_ACTION
   Lock *l = lock_for_pointer(ptr);

 #define OPTIMISED_CASES                                                        \
   OPTIMISED_CASE(1, IS_LOCK_FREE_1, uint8_t)                                   \
   OPTIMISED_CASE(2, IS_LOCK_FREE_2, uint16_t)                                  \
   OPTIMISED_CASE(4, IS_LOCK_FREE_4, uint32_t)                                  \
   OPTIMISED_CASE(8, IS_LOCK_FREE_8, uint64_t)
 #endif
 
 #define OPTIMISED_CASE(n, lockfree, type)                                      \
+  type __atomic_load_##n(type *src, int model);                                \
   type __atomic_load_##n(type *src, int model) {                               \
     if (lockfree)                                                              \
       return __c11_atomic_load((_Atomic(type) *)src, model);                   \
     Lock *l = lock_for_pointer(src);                                           \
     lock(l);                                                                   \
     type val = *src;                                                           \
     unlock(l);                                                                 \
     return val;                                                                \
   }
 OPTIMISED_CASES
 #undef OPTIMISED_CASE
 
 #define OPTIMISED_CASE(n, lockfree, type)                                      \
+  void __atomic_store_##n(type *dest, type val, int model);                    \
   void __atomic_store_##n(type *dest, type val, int model) {                   \
     if (lockfree) {                                                            \
       __c11_atomic_store((_Atomic(type) *)dest, val, model);                   \
       return;                                                                  \
     }                                                                          \
     Lock *l = lock_for_pointer(dest);                                          \
     lock(l);                                                                   \
     *dest = val;                                                               \
     unlock(l);                                                                 \
     return;                                                                    \
   }
 OPTIMISED_CASES
 #undef OPTIMISED_CASE
 
 #define OPTIMISED_CASE(n, lockfree, type)                                      \
+  type __atomic_exchange_##n(type *dest, type val, int model);                 \
   type __atomic_exchange_##n(type *dest, type val, int model) {                \
     if (lockfree)                                                              \
       return __c11_atomic_exchange((_Atomic(type) *)dest, val, model);         \
     Lock *l = lock_for_pointer(dest);                                          \
     lock(l);                                                                   \
     type tmp = *dest;                                                          \
     *dest = val;                                                               \
     unlock(l);                                                                 \
     return tmp;                                                                \
   }
 OPTIMISED_CASES
 #undef OPTIMISED_CASE
 
 #define OPTIMISED_CASE(n, lockfree, type)                                      \
   int __atomic_compare_exchange_##n(type *ptr, type *expected, type desired,   \
+                                    int success, int failure);                 \
+  int __atomic_compare_exchange_##n(type *ptr, type *expected, type desired,   \
                                     int success, int failure) {                \
     if (lockfree)                                                              \
       return __c11_atomic_compare_exchange_strong(                             \
           (_Atomic(type) *)ptr, expected, desired, success, failure);          \
     Lock *l = lock_for_pointer(ptr);                                           \
     lock(l);                                                                   \
     if (*ptr == *expected) {                                                   \
       *ptr = desired;                                                          \
       unlock(l);                                                               \
       return 1;                                                                \
     }                                                                          \
     *expected = *ptr;                                                          \
     unlock(l);                                                                 \
     return 0;                                                                  \
   }
 OPTIMISED_CASES
 #undef OPTIMISED_CASE
 
 ////////////////////////////////////////////////////////////////////////////////
 // Atomic read-modify-write operations for integers of various sizes.
 ////////////////////////////////////////////////////////////////////////////////
 #define ATOMIC_RMW(n, lockfree, type, opname, op)                              \
+  type __atomic_fetch_##opname##_##n(type *ptr, type val, int model);          \
   type __atomic_fetch_##opname##_##n(type *ptr, type val, int model) {         \
     if (lockfree)                                                              \
       return __c11_atomic_fetch_##opname((_Atomic(type) *)ptr, val, model);    \
     Lock *l = lock_for_pointer(ptr);                                           \
     lock(l);                                                                   \
     type tmp = *ptr;                                                           \
     *ptr = tmp op val;                                                         \
     unlock(l);                                                                 \