Fix assertion in ClangASTContext

[labath]

Summary

llvm::APSInt(0) asserts because it creates an int with bit-width 0 and
not (as I thought) a value 0.

Theoretically it should be sufficient to change this to APSInt(1), as
the intention there was that the value of the first argument should be
ignored if the type is invalid, but that would look dodgy.

Instead, I use llvm::Optional to denote an invalid value and use a
special struct instead of a std::pair, to reduce typing and increase
clarity.

[clayborg]

Few nits, but nothing that would hold up the patch. Looks good.

include/lldb/Symbol/CompilerType.h

294 ↗

(On Diff #124781)

Can't you just define the type right here?

source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp

62–63 ↗

(On Diff #124781)

Should we combine these two lines?

if (auto arg = m_backend.GetCompilerType().GetIntegralTemplateArgument(0))

[labath]

include/lldb/Symbol/CompilerType.h

294 ↗

(On Diff #124781)

Unfortunately, I can't because it contains a CompilerType member, which means CompilerType has to be a complete type, and this only happens after we reach the end of the class definition.

However, I may have gone a bit overboard with the nesting of this type inside CompilerType. I could make this just a standalone class if you think that will make things more understandable (a forward declaration will still be needed though).

lldb/trunk/include/lldb/Symbol/ClangASTContext.h

public:
 
   size_t GetNumTemplateArguments(lldb::opaque_compiler_type_t type) override;
 
   lldb::TemplateArgumentKind
   GetTemplateArgumentKind(lldb::opaque_compiler_type_t type,
                           size_t idx) override;
   CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type,
                                        size_t idx) override;
-  std::pair<llvm::APSInt, CompilerType>
+  llvm::Optional<CompilerType::IntegralTemplateArgument>
   GetIntegralTemplateArgument(lldb::opaque_compiler_type_t type,
                               size_t idx) override;
 
   CompilerType GetTypeForFormatters(void *type) override;
 
 #define LLDB_INVALID_DECL_LEVEL UINT32_MAX
   // LLDB_INVALID_DECL_LEVEL is returned by CountDeclLevels if
   // child_decl_ctx could not be found in decl_ctx.

lldb/trunk/include/lldb/Symbol/CompilerType.h

 #include <functional>
 #include <string>
 #include <vector>
 
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Core/ClangForward.h"
 #include "lldb/lldb-private.h"
+#include "llvm/ADT/APSInt.h"
 
 namespace lldb_private {
 
 class DataExtractor;
 
 //----------------------------------------------------------------------
 // A class that can carry around a clang ASTContext and a opaque clang
 // QualType. A clang::QualType can be easily reconstructed from an
public:
   // Create related types using the current type's AST
   //----------------------------------------------------------------------
   CompilerType GetBasicTypeFromAST(lldb::BasicType basic_type) const;
 
   //----------------------------------------------------------------------
   // Exploring the type
   //----------------------------------------------------------------------
 
+  struct IntegralTemplateArgument;
+
   uint64_t GetByteSize(ExecutionContextScope *exe_scope) const;
 
   uint64_t GetBitSize(ExecutionContextScope *exe_scope) const;
 
   lldb::Encoding GetEncoding(uint64_t &count) const;
 
   lldb::Format GetFormat() const;
 
public:
   GetIndexOfChildMemberWithName(const char *name, bool omit_empty_base_classes,
                                 std::vector<uint32_t> &child_indexes) const;
 
   size_t GetNumTemplateArguments() const;
 
   lldb::TemplateArgumentKind GetTemplateArgumentKind(size_t idx) const;
   CompilerType GetTypeTemplateArgument(size_t idx) const;
 
-  // Returns the value of the template argument and its type. In case the
-  // argument is not found, returns an invalid CompilerType.
-  std::pair<llvm::APSInt, CompilerType> GetIntegralTemplateArgument(size_t idx) const;
+  // Returns the value of the template argument and its type.
+  llvm::Optional<IntegralTemplateArgument>
+  GetIntegralTemplateArgument(size_t idx) const;
 
   CompilerType GetTypeForFormatters() const;
 
   LazyBool ShouldPrintAsOneLiner(ValueObject *valobj) const;
 
   bool IsMeaninglessWithoutDynamicResolution() const;
 
   //------------------------------------------------------------------

 private:
   lldb::opaque_compiler_type_t m_type;
   TypeSystem *m_type_system;
 };
 
 bool operator==(const CompilerType &lhs, const CompilerType &rhs);
 bool operator!=(const CompilerType &lhs, const CompilerType &rhs);
 
+struct CompilerType::IntegralTemplateArgument {
+  llvm::APSInt value;
+  CompilerType type;
+};
+
 } // namespace lldb_private
 
 #endif // liblldb_CompilerType_h_

lldb/trunk/include/lldb/Symbol/TypeSystem.h

GetIndexOfChildMemberWithName(lldb::opaque_compiler_type_t type,
                                 std::vector<uint32_t> &child_indexes) = 0;
 
   virtual size_t GetNumTemplateArguments(lldb::opaque_compiler_type_t type) = 0;
 
   virtual lldb::TemplateArgumentKind
   GetTemplateArgumentKind(lldb::opaque_compiler_type_t type, size_t idx);
   virtual CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type,
                                            size_t idx);
-  virtual std::pair<llvm::APSInt, CompilerType>
-  GetIntegralTemplateArgument(lldb::opaque_compiler_type_t type,
-                              size_t idx);
+  virtual llvm::Optional<CompilerType::IntegralTemplateArgument>
+  GetIntegralTemplateArgument(lldb::opaque_compiler_type_t type, size_t idx);
 
   //----------------------------------------------------------------------
   // Dumping types
   //----------------------------------------------------------------------
 
   virtual void DumpValue(lldb::opaque_compiler_type_t type,
                          ExecutionContext *exe_ctx, Stream *s,
                          lldb::Format format, const DataExtractor &data,

lldb/trunk/source/API/SBType.cpp

lldb::SBType SBType::GetTemplateArgumentType(uint32_t idx) {
   CompilerType type;
   switch(GetTemplateArgumentKind(idx)) {
     case eTemplateArgumentKindType:
       type = m_opaque_sp->GetCompilerType(false).GetTypeTemplateArgument(idx);
       break;
     case eTemplateArgumentKindIntegral:
       type = m_opaque_sp->GetCompilerType(false)
                  .GetIntegralTemplateArgument(idx)
-                 .second;
+                 ->type;
       break;
     default:
       break;
   }
   if (type.IsValid())
     return SBType(type);
   return SBType();
 }

lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp

bool BitsetFrontEnd::Update() {
   m_first.reset();
 
   TargetSP target_sp = m_backend.GetTargetSP();
   if (!target_sp)
     return false;
   size_t capping_size = target_sp->GetMaximumNumberOfChildrenToDisplay();
 
   size_t size = 0;
-  auto value_and_type =
-      m_backend.GetCompilerType().GetIntegralTemplateArgument(0);
-  if (value_and_type.second)
-    size = value_and_type.first.getLimitedValue(capping_size);
+  if (auto arg = m_backend.GetCompilerType().GetIntegralTemplateArgument(0))
+    size = arg->value.getLimitedValue(capping_size);
 
   m_elements.assign(size, ValueObjectSP());
 
   m_first = m_backend.GetChildMemberWithName(ConstString("__first_"), true);
   return false;
 }
 
 ValueObjectSP BitsetFrontEnd::GetChildAtIndex(size_t idx) {

lldb/trunk/source/Symbol/ClangASTContext.cpp

ClangASTContext::GetTypeTemplateArgument(lldb::opaque_compiler_type_t type,
   const clang::TemplateArgument &template_arg =
       template_decl->getTemplateArgs()[idx];
   if (template_arg.getKind() != clang::TemplateArgument::Type)
     return CompilerType();
 
   return CompilerType(getASTContext(), template_arg.getAsType());
 }
 
-std::pair<llvm::APSInt, CompilerType>
+llvm::Optional<CompilerType::IntegralTemplateArgument>
 ClangASTContext::GetIntegralTemplateArgument(lldb::opaque_compiler_type_t type,
                                              size_t idx) {
   const clang::ClassTemplateSpecializationDecl *template_decl =
       GetAsTemplateSpecialization(type);
   if (! template_decl || idx >= template_decl->getTemplateArgs().size())
-    return {llvm::APSInt(0), CompilerType()};
+    return llvm::None;
 
   const clang::TemplateArgument &template_arg =
       template_decl->getTemplateArgs()[idx];
   if (template_arg.getKind() != clang::TemplateArgument::Integral)
-    return {llvm::APSInt(0), CompilerType()};
+    return llvm::None;
 
-  return {template_arg.getAsIntegral(),
-          CompilerType(getASTContext(), template_arg.getIntegralType())};
+  return {{template_arg.getAsIntegral(),
+           CompilerType(getASTContext(), template_arg.getIntegralType())}};
 }
 
 CompilerType ClangASTContext::GetTypeForFormatters(void *type) {
   if (type)
     return ClangUtil::RemoveFastQualifiers(CompilerType(this, type));
   return CompilerType();
 }
 

lldb/trunk/source/Symbol/CompilerType.cpp

 
 CompilerType CompilerType::GetTypeTemplateArgument(size_t idx) const {
   if (IsValid()) {
     return m_type_system->GetTypeTemplateArgument(m_type, idx);
   }
   return CompilerType();
 }
 
-std::pair<llvm::APSInt, CompilerType>
-CompilerType::GetIntegralTemplateArgument(size_t idx) const
-{
+llvm::Optional<CompilerType::IntegralTemplateArgument>
+CompilerType::GetIntegralTemplateArgument(size_t idx) const {
   if (IsValid())
     return m_type_system->GetIntegralTemplateArgument(m_type, idx);
-  return {llvm::APSInt(0), CompilerType()};
+  return llvm::None;
 }
 
 CompilerType CompilerType::GetTypeForFormatters() const {
   if (IsValid())
     return m_type_system->GetTypeForFormatters(m_type);
   return CompilerType();
 }
 

lldb/trunk/source/Symbol/TypeSystem.cpp

TypeSystem::GetTemplateArgumentKind(opaque_compiler_type_t type, size_t idx) {
   return eTemplateArgumentKindNull;
 }
 
 CompilerType TypeSystem::GetTypeTemplateArgument(opaque_compiler_type_t type,
                                                  size_t idx) {
   return CompilerType();
 }
 
-std::pair<llvm::APSInt, CompilerType>
+llvm::Optional<CompilerType::IntegralTemplateArgument>
 TypeSystem::GetIntegralTemplateArgument(opaque_compiler_type_t type,
                                         size_t idx) {
-  return {llvm::APSInt(0), CompilerType()};
+  return llvm::None;
 }
 
 LazyBool TypeSystem::ShouldPrintAsOneLiner(void *type, ValueObject *valobj) {
   return eLazyBoolCalculate;
 }
 
 bool TypeSystem::IsMeaninglessWithoutDynamicResolution(void *type) {
   return false;

lldb/trunk/unittests/Symbol/TestClangASTContext.cpp

EXPECT_TRUE(
       ClangASTContext::RecordHasFields(empty_derived_non_empty_vbase_decl));
 }
 
 TEST_F(TestClangASTContext, TemplateArguments) {
   ClangASTContext::TemplateParameterInfos infos;
   infos.names.push_back("T");
   infos.args.push_back(TemplateArgument(m_ast->getASTContext()->IntTy));
   infos.names.push_back("I");
-  infos.args.push_back(TemplateArgument(*m_ast->getASTContext(),
-                                        llvm::APSInt(47),
+  llvm::APSInt arg(llvm::APInt(8, 47));
+  infos.args.push_back(TemplateArgument(*m_ast->getASTContext(), arg,
                                         m_ast->getASTContext()->IntTy));
 
   // template<typename T, int I> struct foo;
   ClassTemplateDecl *decl = m_ast->CreateClassTemplateDecl(
       m_ast->GetTranslationUnitDecl(), eAccessPublic, "foo", TTK_Struct, infos);
   ASSERT_NE(decl, nullptr);
 
   // foo<int, 47>
TEST_F(TestClangASTContext, TemplateArguments) {
   CompilerType int_type(m_ast->getASTContext(), m_ast->getASTContext()->IntTy);
   for(CompilerType t: { type, typedef_type, auto_type }) {
     SCOPED_TRACE(t.GetTypeName().AsCString());
 
     EXPECT_EQ(m_ast->GetTemplateArgumentKind(t.GetOpaqueQualType(), 0),
               eTemplateArgumentKindType);
     EXPECT_EQ(m_ast->GetTypeTemplateArgument(t.GetOpaqueQualType(), 0),
               int_type);
-    auto p = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 0);
-    EXPECT_EQ(p.second, CompilerType());
+    EXPECT_EQ(llvm::None,
+              m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 0));
 
     EXPECT_EQ(m_ast->GetTemplateArgumentKind(t.GetOpaqueQualType(), 1),
               eTemplateArgumentKindIntegral);
     EXPECT_EQ(m_ast->GetTypeTemplateArgument(t.GetOpaqueQualType(), 1),
               CompilerType());
-    p = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 1);
-    EXPECT_EQ(p.first, llvm::APSInt(47));
-    EXPECT_EQ(p.second, int_type);
+    auto result = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 1);
+    ASSERT_NE(llvm::None, result);
+    EXPECT_EQ(arg, result->value);
+    EXPECT_EQ(int_type, result->type);
   }
 }