[lldb] Add unreachable after fully covered switches, avoid GCC warnings. NFC.

[mstorsjo]

This avoids the following kind of warning with GCC:

warning: control reaches end of non-void function [-Wreturn-type]

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Martin Storsjö (mstorsjo)

Changes

This avoids the following kind of warning with GCC:

warning: control reaches end of non-void function [-Wreturn-type]

---

Full diff: https://github.com/llvm/llvm-project/pull/159327.diff

4 Files Affected:

• (modified) lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp (+1)
• (modified) lldb/source/Plugins/Process/Utility/HistoryUnwind.cpp (+1)
• (modified) lldb/source/Plugins/Process/wasm/ProcessWasm.cpp (+1)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+2)

diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
index 1f7b8d48d0fc8..4e8a430af8c6c 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -2199,6 +2199,7 @@ bool CPlusPlusLanguage::GetFunctionDisplayName(
   case FunctionNameRepresentation::eName:
     return false;
   }
+  llvm_unreachable("Fully covered switch above");
 }
 
 bool CPlusPlusLanguage::HandleFrameFormatVariable(
diff --git a/lldb/source/Plugins/Process/Utility/HistoryUnwind.cpp b/lldb/source/Plugins/Process/Utility/HistoryUnwind.cpp
index 3b0618fa10374..1204faf4fa61e 100644
--- a/lldb/source/Plugins/Process/Utility/HistoryUnwind.cpp
+++ b/lldb/source/Plugins/Process/Utility/HistoryUnwind.cpp
@@ -60,6 +60,7 @@ static bool BehavesLikeZerothFrame(HistoryPCType pc_type, uint32_t frame_idx) {
   case HistoryPCType::Calls:
     return true;
   }
+  llvm_unreachable("Fully covered switch above");
 }
 
 bool HistoryUnwind::DoGetFrameInfoAtIndex(uint32_t frame_idx, lldb::addr_t &cfa,
diff --git a/lldb/source/Plugins/Process/wasm/ProcessWasm.cpp b/lldb/source/Plugins/Process/wasm/ProcessWasm.cpp
index 580e8c1d9cfa4..62bcf442d097a 100644
--- a/lldb/source/Plugins/Process/wasm/ProcessWasm.cpp
+++ b/lldb/source/Plugins/Process/wasm/ProcessWasm.cpp
@@ -98,6 +98,7 @@ size_t ProcessWasm::ReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
         "Wasm read failed for invalid address 0x%" PRIx64, vm_addr);
     return 0;
   }
+  llvm_unreachable("Fully covered switch above");
 }
 
 llvm::Expected<std::vector<lldb::addr_t>>
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 5801fa97bf753..881268bc4ca03 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2502,6 +2502,7 @@ static llvm::StringRef ClangToItaniumCtorKind(clang::CXXCtorType kind) {
   case clang::CXXCtorType::Ctor_Comdat:
     llvm_unreachable("Unexpected constructor kind.");
   }
+  llvm_unreachable("Fully covered switch above");
 }
 
 static llvm::StringRef ClangToItaniumDtorKind(clang::CXXDtorType kind) {
@@ -2517,6 +2518,7 @@ static llvm::StringRef ClangToItaniumDtorKind(clang::CXXDtorType kind) {
   case clang::CXXDtorType::Dtor_Comdat:
     llvm_unreachable("Unexpected destructor kind.");
   }
+  llvm_unreachable("Fully covered switch above");
 }
 
 static llvm::StringRef

[DavidSpickett]

Taking one as an example:

  enum class FunctionNameRepresentation {
    eName,
    eNameWithArgs,
    eNameWithNoArgs
  };

And we do have a return for each of the cases.

So it feels like a limitation in GCC's analysis, is that right or did they make a decision to warn in these cases? I can see some value in making people mark the "unused" exit path like you might a fallthrough case.

If you have some background on it please add it to the PR description. I'll probably want to cite it again at some point :)

[DavidSpickett]

Also if we added an entry to one of these enums, would the "switch doesn't cover value" warning still happen? I think it would right?

[mstorsjo]

Taking one as an example:

  enum class FunctionNameRepresentation {
    eName,
    eNameWithArgs,
    eNameWithNoArgs
  };

And we do have a return for each of the cases.

So it feels like a limitation in GCC's analysis, is that right or did they make a decision to warn in these cases? I can see some value in making people mark the "unused" exit path like you might a fallthrough case.

See https://github.com/llvm/llvm-project/blob/main/llvm/docs/CodingStandards.rst#don-t-use-default-labels-in-fully-covered-switches-over-enumerations - the root cause here is that GCC assumes that an enum variable still technically can have any value outside of the enum, while Clang is ok with it.

If you have some background on it please add it to the PR description. I'll probably want to cite it again at some point :)

Also if we added an entry to one of these enums, would the "switch doesn't cover value" warning still happen? I think it would right?

Yes, we separately use -Wswitch which does produce this kind of warning with both GCC and Clang if we're missing enum elements in the switch:

../../lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:2181:11: warning: enumeration value 'eName' not handled in switch [-Wswitch]
 2181 |   switch (representation) {
      |           ^~~~~~~~~~~~~~

[DavidSpickett]

the root cause here is that GCC assumes that an enum variable still technically can have any value outside of the enum

Ok, I forgot about this aspect.

LGTM.