[MC] Add llvm_unreachable to toString to fix compile time warning.

[fhahn]

Summary

Without this change, GCC 7 raises the warning below:

control reaches end of non-void function

[andreadb]

I just saw that Gabor fixed it at revision 331251 adding a 'default' case.
http://llvm.org/viewvc/llvm-project?view=revision&revision=331251

[fhahn]

In D46304#1083822, @andreadb wrote:

I just saw that Gabor fixed it at revision 331251 adding a 'default' case.
http://llvm.org/viewvc/llvm-project?view=revision&revision=331251

Ah thanks for pointing me to that :) @GBuella I think one minor drawback of adding a default cast is that we won't get compiler warning for missing cases, but then when WasmSymbolType is not an enum class, you probably won't get them anyways.

[andreadb]

Your approach is better.

Adding a default case is not a good workaround because it introduces another warning:

/Users/buildslave/as-bldslv9/lld-x86_64-darwin13/llvm.src/lib/MC/WasmObjectWriter.cpp:50:3: error: default label in switch which covers all enumeration values [-Werror,-Wcovered-switch-default]

default:
^

Can you push your patch to fix the buildbots?

Cheers,
Andrea

[GBuella]

In D46304#1083824, @fhahn wrote:

In D46304#1083822, @andreadb wrote:

I just saw that Gabor fixed it at revision 331251 adding a 'default' case.
http://llvm.org/viewvc/llvm-project?view=revision&revision=331251

Ah thanks for pointing me to that :) @GBuella I think one minor drawback of adding a default cast is that we won't get compiler warning for missing cases, but then when WasmSymbolType is not an enum class, you probably won't get them anyways.

Thanks for the fix!
Yes, since enum is defined as "enum WasmSymbolType : unsigned { ... }", the enumeration values are [0 ... UINT_MAX], as ISO says:
"For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type."

So the wording of this warning is definitely wrong (unless there would be cases for 2^32 enumeration values):

llvm/lib/MC/WasmObjectWriter.cpp:50:3: error: default label in switch which covers all enumeration values [-Werror,-Wcovered-switch-default]
  default:
  ^

[dblaikie]

In D46304#1084947, @GBuella wrote:

In D46304#1083824, @fhahn wrote:

In D46304#1083822, @andreadb wrote:

I just saw that Gabor fixed it at revision 331251 adding a 'default' case.
http://llvm.org/viewvc/llvm-project?view=revision&revision=331251

Ah thanks for pointing me to that :) @GBuella I think one minor drawback of adding a default cast is that we won't get compiler warning for missing cases, but then when WasmSymbolType is not an enum class, you probably won't get them anyways.

Thanks for the fix!
Yes, since enum is defined as "enum WasmSymbolType : unsigned { ... }", the enumeration values are [0 ... UINT_MAX], as ISO says:
"For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type."

So the wording of this warning is definitely wrong (unless there would be cases for 2^32 enumeration values):

llvm/lib/MC/WasmObjectWriter.cpp:50:3: error: default label in switch which covers all enumeration values [-Werror,-Wcovered-switch-default]
  default:
  ^

Hey there - I implemented this diagnostic many a year ago. Yeah, technically maybe "covers all enumerators" might be more accurate phrasing?

[GBuella]

Hey there - I implemented this diagnostic many a year ago. Yeah, technically maybe "covers all enumerators" might be more accurate phrasing?

I'm not sure. I would rather enable this warning only on enums without fixed type, i.e. "enum XX {}", and not use this warning with "enum XX : unsigned {}". Or these could be two distinct warnings.
If someone explicitly fixes the type of an enum this way, then one might expect such "other values" to appear sometimes.

E.g. I guess this WasmSymbolType is marked as unsigned, because it might be read from a binary file, in which case that input might contain invalid values, by which I values not matching the enumerated enumeration values. This is probably already validated before this toString function is called, but still.

[sbc100]

In D46304#1091019, @GBuella wrote:

Hey there - I implemented this diagnostic many a year ago. Yeah, technically maybe "covers all enumerators" might be more accurate phrasing?

I'm not sure. I would rather enable this warning only on enums without fixed type, i.e. "enum XX {}", and not use this warning with "enum XX : unsigned {}". Or these could be two distinct warnings.
If someone explicitly fixes the type of an enum this way, then one might expect such "other values" to appear sometimes.

E.g. I guess this WasmSymbolType is marked as unsigned, because it might be read from a binary file, in which case that input might contain invalid values, by which I values not matching the enumerated enumeration values. This is probably already validated before this toString function is called, but still.

The wording says the values of the enumeration are the values of the underlying type. I think perhaps that wording is a little wrong. Perhaps the final the should be removed? Surly what it trying to say is that is that the valid enum values are themselves values of the underlying type, not that *all* possible value of the underlying type are also valid enum values.

[GBuella]

The wording says the values of the enumeration are the values of the underlying type. I think perhaps that wording is a little wrong. Perhaps the final the should be removed? Surly what it trying to say is that is that the valid enum values are themselves values of the underlying type, not that *all* possible value of the underlying type are also valid enum values.

@sbc100, just read after that sentence:
"Otherwise, for an enumeration where emin is the smallest enumerator and emax is the largest, the values of the enumeration are the values in the range bmin to bmax, ..."

So, for one without a fixed type, the way I would interpret it, the following type has 7 valid values (0, 1, 2, 3, 4, 5, 6, 7):

enum x {
  a = 2
  b = 6
};

A bit later on it is explicitly stated, that:
"It is possible to define an enumeration that has values not defined by any of its enumerators"
Therefore, even for such simple enum without a fixed type, a default case can be useful sometimes.

And I believe in the case of those with fixed types, the range of the "values of the enumeration" is just extended to *all* possible values of that underlying type -- well, that would make sense to me.

BTW, yes, "covers all enumerators" might be more accurate phrasing -- that surely is OK for all kinds of C++ enums, and for C enums.

llvm/trunk/lib/MC/WasmObjectWriter.cpp

static std::string toString(wasm::WasmSymbolType type) {
   case wasm::WASM_SYMBOL_TYPE_FUNCTION:
     return "WASM_SYMBOL_TYPE_FUNCTION";
   case wasm::WASM_SYMBOL_TYPE_GLOBAL:
     return "WASM_SYMBOL_TYPE_GLOBAL";
   case wasm::WASM_SYMBOL_TYPE_DATA:
     return "WASM_SYMBOL_TYPE_DATA";
   case wasm::WASM_SYMBOL_TYPE_SECTION:
     return "WASM_SYMBOL_TYPE_SECTION";
-  default:
-    llvm_unreachable("unexpected kind");
   }
+  llvm_unreachable("unknown symbol type");
 }
 
 static std::string relocTypetoString(uint32_t type) {
   switch (type) {
 #define WASM_RELOC(NAME, VALUE) case VALUE: return #NAME;
 #include "llvm/BinaryFormat/WasmRelocs.def"
 #undef WASM_RELOC
   default: