dwarf::isCPlusPlus fails with user language

[waj]

Summary

As described in https://bugs.llvm.org/show_bug.cgi?id=46027, this function is currently failing for language codes falling between DW_LANG_lo_user and DW_LANG_hi_user

[waj]

This was already tested by Homebrew team (https://github.com/crystal-lang/crystal/issues/9148#issuecomment-632896285) and seems to be working correctly

[jmorse]

@waj Could you upload the diff with context added (add -U99999 to your diff command). I should have linked you to the docs on uploading reviews sorry, which are here: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

I've added some DWARFy reviewers. The change seems reasonable to me: we accept any language number in the correct range in DIBuilder::createCompileUnit, so it seems odd to then abort due to illegal state in isCPlusPlus, unless there's some underlying assumption that I'm not familiar with.

[probinson]

FTR, we like to have full context diffs as described here: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

The code patch looks fine. However, we want tests for all new features, but the patch introducing isCPlusPlus doesn't seem to have included an explicit test, and making you write one seems excessive. + @dblaikie as he committed that faux pas.

[aprantl]

Without the context it's difficult to tell, but I'd image that you might be able to add a tiny unit test for this. The code change itself LGTM in the sense of that it is strictly better than crashing, however, I wonder what that means if someone adds a custom C++-based dialect in the user range. I guess they'll need to update this function then.

[aprantl]

It would have been nice if the standard had assigned the DW_LANG constants in a DW_LANG_cplus_plus | version << offset fashion.

[dblaikie]

In D81389#2079773, @probinson wrote:

FTR, we like to have full context diffs as described here: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

The code patch looks fine. However, we want tests for all new features, but the patch introducing isCPlusPlus doesn't seem to have included an explicit test, and making you write one seems excessive. + @dblaikie as he committed that faux pas.

Not sure what an 'explicit' test for this would look like - the function is used to determine a few things about how to handle composite names (namespaces, etc) - whether to use '::' as a path separator (llvm::DwarfUnit::getParentContextString). I'm generally happy for tests to cover unions of different behavior sometimes - if it doesn't convolute the test too much.

In any case, yeah, this change does need test coverage - if folks are using LLVM debug info for non-C++ languages and using pubnames or debug_names (which is what you'd have to be using to trip over this unreachable) you're probably going to want to look at llvm::DwarfUnit::getParentContextString that doesn't add any context for non-C++ scopes. So if your language has scoping this might be a problem for debug info consumers.

If all of that is fine/correct - then, yeah, a small test case using pubnames or debug_names that shows the namespace scopes are not included in the names of nested elements in the index would be good.

llvm/include/llvm/BinaryFormat/Dwarf.h

   case DW_LANG_hi_user:
     return false;
   }
+  if (S >= DW_LANG_lo_user && S <= DW_LANG_hi_user)
+    return false;
   llvm_unreachable("Invalid source language");
 }