Wrap `llvm_unreachable` macro in do-while loop

[sgraenitz]

Summary

Macros that expand into multiple terms can cause interesting preprocessor hickups depending
on the context they are used in. https://github.com/llvm/llvm-project/issues/56867 reported
a miscompilation of llvm_unreachable(msg) inside a LLVM_DEBUG({ ... }) block. We were
able to fix it by wrapping the expansion in a do {} while(false).

[sgraenitz]

Please find more details on the diagnosis here https://github.com/llvm/llvm-project/issues/56867#issuecomment-1205190498

llvm/include/llvm/Support/ErrorHandling.h

152

One might argue that macros should always expand into the same expression type. Not sure it's necessary here, but of course we could apply this change to the other cases as well. Let me know if you think it would be better.

[barannikov88]

llvm/include/llvm/Support/ErrorHandling.h

150–154

This is improperly formatted, please use clang-format.

[sgraenitz]

llvm/include/llvm/Support/ErrorHandling.h

150–154

Right, clang-format want's this:

do { \
  LLVM_BUILTIN_TRAP; \
  LLVM_BUILTIN_UNREACHABLE; \
} while (false)

I choose not to follow it in my patch, because the existing code is only 6 months old and didn't respect clang-format. Instead I mimicked the style of the LLVM_DEBUG macro which expands into a similar do-while construct: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Debug.h#L64

Of course, I can change that if there are good reasons or strong opinions on it.

[barannikov88]

llvm/include/llvm/Support/ErrorHandling.h

150–154

Imagine that the 6-month old change followed the style. It would save your time on deciding whether to clang-format it or not. Formatting it properly now will save people time when modifying this file in the future. This is the whole purpose of the style guide -- saving time on choosing the right formatting.
I can't disagree that consistency is more important than style, but LLVM_DEBUG is not even in this file.

[mehdi_amini]

llvm/include/llvm/Support/ErrorHandling.h

150–154

FYI: I find the clang-format version much more readable, it's not clear to me why we shouldn't just adopt it.

[sgraenitz]

clang-format

[barannikov88]

LGTM, thanks

llvm/include/llvm/Support/ErrorHandling.h

 #ifndef NDEBUG
 #define llvm_unreachable(msg) \
   ::llvm::llvm_unreachable_internal(msg, __FILE__, __LINE__)
 #elif !defined(LLVM_BUILTIN_UNREACHABLE)
 #define llvm_unreachable(msg) ::llvm::llvm_unreachable_internal()
 #elif LLVM_UNREACHABLE_OPTIMIZE
 #define llvm_unreachable(msg) LLVM_BUILTIN_UNREACHABLE
 #else
-#define llvm_unreachable(msg) LLVM_BUILTIN_TRAP, LLVM_BUILTIN_UNREACHABLE
+#define llvm_unreachable(msg)                                                  \
+  do {                                                                         \
+    LLVM_BUILTIN_TRAP;                                                         \
+    LLVM_BUILTIN_UNREACHABLE;                                                  \
+  } while (false)
 #endif
 
 #endif