llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-backend-mips Author: Michael Buch (Michael137) <details> <summary>Changes</summary> This patch re-implements `TypeSystemClang::IsFloatingPointType` by forwarding it to `clang::Type::isFloatingType`. The main difference is that the latter returns false for float vector types. Users of this API should test for vectors explicitly (or use `GetTypeInfo`). Otherwise this is a foot-gun because most callsites probably don't consider treating float vector types. This patch makes all the callers of `IsFloatingPointType` now check `GetTypeInfo() & eTypeIsFloat`. This is set for float vector types too, so behaviour doesn't change. To make sure we audit all the call-sites in `ValueObject.cpp`, I added a helper `HasFloatRepresentation` (named after the `clang::Type::hasFloatRepresentation` API), which does the `GetTypeInfo` check, and added a FIXME to it. --- Full diff: https://github.com/llvm/llvm-project/pull/179213.diff 10 Files Affected: - (modified) lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp (+2-4) - (modified) lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp (+2-2) - (modified) lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp (+1-1) - (modified) lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp (+3-2) - (modified) lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp (+4-2) - (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+6-20) - (modified) lldb/source/ValueObject/DILEval.cpp (+3-2) - (modified) lldb/source/ValueObject/ValueObject.cpp (+18-10) - (modified) lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py (+10-5) - (modified) lldb/unittests/Symbol/TestTypeSystemClang.cpp (+34-5) ``````````diff diff --git a/lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp b/lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp index 8c54159965f3c..5b6cdbf74504c 100644 --- a/lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp +++ b/lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp @@ -1633,7 +1633,6 @@ ValueObjectSP ABISysV_arm::GetReturnValueObjectImpl( return return_valobj_sp; } } else if (compiler_type.IsFloatingPointType()) { - // Vector types are handled above. if (!compiler_type.IsCompleteType()) { switch (*bit_width) { default: @@ -1707,8 +1706,6 @@ ValueObjectSP ABISysV_arm::GetReturnValueObjectImpl( : homogeneous_count * 2); } } else if (base_type.IsFloatingPointType()) { - assert(!base_type.IsVectorType() && - "Vector types should've been handled above."); if (!base_type.IsComplexType()) { is_vfp_candidate = true; if (base_byte_size) @@ -1726,7 +1723,8 @@ ValueObjectSP ABISysV_arm::GetReturnValueObjectImpl( base_type = compiler_type.GetFieldAtIndex(index, name, nullptr, nullptr, nullptr); - if (base_type.IsFloatingPointType()) { + // TODO: is this correct for float vector types? + if (base_type.GetTypeInfo() & eTypeIsFloat) { std::optional<uint64_t> base_byte_size = llvm::expectedToOptional(base_type.GetByteSize(&thread)); if (base_type.IsComplexType()) { diff --git a/lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp b/lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp index af71f4ad08342..2cd49478878eb 100644 --- a/lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp +++ b/lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp @@ -935,7 +935,7 @@ ValueObjectSP ABISysV_mips64::GetReturnValueObjectImpl( return_compiler_type.GetFieldAtIndex(idx, name, &field_bit_offset, nullptr, nullptr); - if (field_compiler_type.IsFloatingPointType()) + if (field_compiler_type.GetTypeInfo() & eTypeIsFloat) use_fp_regs = true; else found_non_fp_field = true; @@ -1042,7 +1042,7 @@ ValueObjectSP ABISysV_mips64::GetReturnValueObjectImpl( if (field_compiler_type.IsIntegerOrEnumerationType(is_signed) || field_compiler_type.IsPointerType() || - field_compiler_type.IsFloatingPointType()) { + field_compiler_type.GetTypeInfo() & eTypeIsFloat) { padding = field_byte_offset - integer_bytes; if (integer_bytes < 8) { diff --git a/lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp b/lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp index 604a6d6ee9c16..fea5174e8fbba 100644 --- a/lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp +++ b/lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp @@ -732,7 +732,7 @@ ValueObjectSP ABISysV_ppc::GetReturnValueObjectImpl( // return a nullptr return value object. return return_valobj_sp; } - } else if (field_compiler_type.IsFloatingPointType()) { + } else if (field_compiler_type.GetTypeInfo() & eTypeIsFloat) { // Structs with long doubles are always passed in memory. if (*field_bit_width == 128) { is_memory = true; diff --git a/lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp b/lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp index bb19545d14165..1d184d64f5a4e 100644 --- a/lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp +++ b/lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp @@ -596,7 +596,8 @@ static bool FlattenAggregateType( const uint32_t field_type_flags = field_compiler_type.GetTypeInfo(); if (field_compiler_type.IsIntegerOrEnumerationType(is_signed) || field_compiler_type.IsPointerType() || - field_compiler_type.IsFloatingPointType()) { + // FIXME: is this correct for complex floats or float vector types? + field_type_flags & eTypeIsFloat) { aggregate_field_offsets.push_back(field_byte_offset); aggregate_compiler_types.push_back(field_compiler_type); } else if (field_type_flags & eTypeHasChildren) { @@ -724,7 +725,7 @@ ValueObjectSP ABISysV_x86_64::GetReturnValueObjectImpl( // return a nullptr return value object. return return_valobj_sp; } - } else if (field_compiler_type.IsFloatingPointType()) { + } else if (field_compiler_type.GetTypeInfo() & eTypeIsFloat) { // Structs with long doubles are always passed in memory. if (field_bit_width == 128) { is_memory = true; diff --git a/lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp b/lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp index 05498d0b36448..620a247be547e 100644 --- a/lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp +++ b/lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp @@ -572,7 +572,8 @@ static bool FlattenAggregateType( const uint32_t field_type_flags = field_compiler_type.GetTypeInfo(); if (field_compiler_type.IsIntegerOrEnumerationType(is_signed) || field_compiler_type.IsPointerType() || - field_compiler_type.IsFloatingPointType()) { + // FIXME: is this correct for complex floats or float vector types? + field_type_flags & eTypeIsFloat) { aggregate_field_offsets.push_back(field_byte_offset); aggregate_compiler_types.push_back(field_compiler_type); } else if (field_type_flags & eTypeHasChildren) { @@ -679,7 +680,8 @@ ValueObjectSP ABIWindows_x86_64::GetReturnValueObjectImpl( uint32_t copy_from_offset = 0; if (field_compiler_type.IsIntegerOrEnumerationType(is_signed) || field_compiler_type.IsPointerType() || - field_compiler_type.IsFloatingPointType()) { + // FIXME: is this correct for complex floats or float vector types? + field_compiler_type.GetTypeInfo() & eTypeIsFloat) { copy_from_extractor = &rax_data; copy_from_offset = used_bytes; used_bytes += field_byte_width; diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp index 812b4508f4937..c9192938fca80 100644 --- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp +++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp @@ -3474,28 +3474,14 @@ bool TypeSystemClang::IsReferenceType(lldb::opaque_compiler_type_t type, } bool TypeSystemClang::IsFloatingPointType(lldb::opaque_compiler_type_t type) { - if (type) { - clang::QualType qual_type(GetCanonicalQualType(type)); + if (!type) + return false; - if (const clang::BuiltinType *BT = llvm::dyn_cast<clang::BuiltinType>( - qual_type->getCanonicalTypeInternal())) { - clang::BuiltinType::Kind kind = BT->getKind(); - if (kind >= clang::BuiltinType::Float && - kind <= clang::BuiltinType::LongDouble) - return true; - } else if (const clang::ComplexType *CT = - llvm::dyn_cast<clang::ComplexType>( - qual_type->getCanonicalTypeInternal())) { - if (IsFloatingPointType(CT->getElementType().getAsOpaquePtr())) - return true; - } else if (const clang::VectorType *VT = llvm::dyn_cast<clang::VectorType>( - qual_type->getCanonicalTypeInternal())) { - if (IsFloatingPointType(VT->getElementType().getAsOpaquePtr())) - return true; - } - } + clang::QualType qual_type(GetCanonicalQualType(type)); + if (qual_type.isNull()) + return false; - return false; + return qual_type->isFloatingType(); } bool TypeSystemClang::IsDefined(lldb::opaque_compiler_type_t type) { diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp index 5077d8b1aa647..48aa4af1d9fd2 100644 --- a/lldb/source/ValueObject/DILEval.cpp +++ b/lldb/source/ValueObject/DILEval.cpp @@ -814,7 +814,7 @@ Interpreter::VerifyArithmeticCast(CompilerType source_type, CompilerType target_type, int location) { if (source_type.IsPointerType() || source_type.IsNullPtrType()) { // Cast from pointer to float/double is not allowed. - if (target_type.IsFloatingPointType()) { + if (target_type.GetTypeInfo() & lldb::eTypeIsFloat) { std::string errMsg = llvm::formatv("Cast from {0} to {1} is not allowed", source_type.TypeDescription(), target_type.TypeDescription()); @@ -951,7 +951,8 @@ llvm::Expected<lldb::ValueObjectSP> Interpreter::Visit(const CastNode &node) { switch (cast_kind) { case CastKind::eEnumeration: { - if (op_type.IsFloatingPointType() || op_type.IsInteger() || + // FIXME: is this correct for float vector types? + if (op_type.GetTypeInfo() & lldb::eTypeIsFloat || op_type.IsInteger() || op_type.IsEnumerationType()) return operand->CastToEnumType(target_type); break; diff --git a/lldb/source/ValueObject/ValueObject.cpp b/lldb/source/ValueObject/ValueObject.cpp index 4bc7ad17cd257..a804ade9a53c5 100644 --- a/lldb/source/ValueObject/ValueObject.cpp +++ b/lldb/source/ValueObject/ValueObject.cpp @@ -48,7 +48,7 @@ #include "lldb/ValueObject/ValueObjectMemory.h" #include "lldb/ValueObject/ValueObjectSynthetic.h" #include "lldb/ValueObject/ValueObjectVTable.h" -#include "lldb/lldb-private-types.h" +#include "lldb/lldb-enumerations.h" #include "llvm/Support/Compiler.h" @@ -76,6 +76,14 @@ using namespace lldb_private; static user_id_t g_value_obj_uid = 0; +// FIXME: this will return true for vector types whose elements +// are floats. Audit all usages of this function and call +// IsFloatingPointType() instead if vectors of floats aren't intended +// to be supported. +static bool HasFloatRepresentation(CompilerType ct) { + return ct.GetTypeInfo() & eTypeIsFloat; +} + // ValueObject constructor ValueObject::ValueObject(ValueObject &parent) : m_parent(&parent), m_update_point(parent.GetUpdatePoint()), @@ -1165,7 +1173,7 @@ llvm::Expected<llvm::APSInt> ValueObject::GetValueAsAPSInt() { } llvm::Expected<llvm::APFloat> ValueObject::GetValueAsAPFloat() { - if (!GetCompilerType().IsFloatingPointType()) + if (!HasFloatRepresentation(GetCompilerType())) return llvm::make_error<llvm::StringError>( "type cannot be converted to APFloat", llvm::inconvertibleErrorCode()); @@ -1188,7 +1196,7 @@ llvm::Expected<bool> ValueObject::GetValueAsBool() { if (value_or_err) return value_or_err->getBoolValue(); } - if (val_type.IsFloatingPointType()) { + if (HasFloatRepresentation(val_type)) { auto value_or_err = GetValueAsAPFloat(); if (value_or_err) return value_or_err->isNonZero(); @@ -1204,7 +1212,7 @@ void ValueObject::SetValueFromInteger(const llvm::APInt &value, Status &error) { // Verify the current object is an integer object CompilerType val_type = GetCompilerType(); if (!val_type.IsInteger() && !val_type.IsUnscopedEnumerationType() && - !val_type.IsFloatingPointType() && !val_type.IsPointerType() && + !HasFloatRepresentation(val_type) && !val_type.IsPointerType() && !val_type.IsScalarType()) { error = Status::FromErrorString("current value object is not an integer objet"); @@ -1244,7 +1252,7 @@ void ValueObject::SetValueFromInteger(lldb::ValueObjectSP new_val_sp, // Verify the current object is an integer object CompilerType val_type = GetCompilerType(); if (!val_type.IsInteger() && !val_type.IsUnscopedEnumerationType() && - !val_type.IsFloatingPointType() && !val_type.IsPointerType() && + !HasFloatRepresentation(val_type) && !val_type.IsPointerType() && !val_type.IsScalarType()) { error = Status::FromErrorString("current value object is not an integer objet"); @@ -1261,7 +1269,7 @@ void ValueObject::SetValueFromInteger(lldb::ValueObjectSP new_val_sp, // Verify the proposed new value is the right type. CompilerType new_val_type = new_val_sp->GetCompilerType(); - if (!new_val_type.IsInteger() && !new_val_type.IsFloatingPointType() && + if (!new_val_type.IsInteger() && !HasFloatRepresentation(new_val_type) && !new_val_type.IsPointerType()) { error = Status::FromErrorString( "illegal argument: new value should be of the same size"); @@ -1274,7 +1282,7 @@ void ValueObject::SetValueFromInteger(lldb::ValueObjectSP new_val_sp, SetValueFromInteger(*value_or_err, error); else error = Status::FromErrorString("error getting APSInt from new_val_sp"); - } else if (new_val_type.IsFloatingPointType()) { + } else if (HasFloatRepresentation(new_val_type)) { auto value_or_err = new_val_sp->GetValueAsAPFloat(); if (value_or_err) SetValueFromInteger(value_or_err->bitcastToAPInt(), error); @@ -3142,7 +3150,7 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) { bool is_enum = GetCompilerType().IsEnumerationType(); bool is_pointer = GetCompilerType().IsPointerType() || GetCompilerType().IsNullPtrType(); - bool is_float = GetCompilerType().IsFloatingPointType(); + bool is_float = HasFloatRepresentation(GetCompilerType()); bool is_integer = GetCompilerType().IsInteger(); ExecutionContext exe_ctx(GetExecutionContextRef()); @@ -3234,7 +3242,7 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) { } } - if (type.IsFloatingPointType()) { + if (HasFloatRepresentation(type)) { if (!is_scalar) { auto int_value_or_err = GetValueAsAPSInt(); if (int_value_or_err) { @@ -3296,7 +3304,7 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) { lldb::ValueObjectSP ValueObject::CastToEnumType(CompilerType type) { bool is_enum = GetCompilerType().IsEnumerationType(); bool is_integer = GetCompilerType().IsInteger(); - bool is_float = GetCompilerType().IsFloatingPointType(); + bool is_float = HasFloatRepresentation(GetCompilerType()); ExecutionContext exe_ctx(GetExecutionContextRef()); if (!is_enum && !is_integer && !is_float) diff --git a/lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py b/lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py index 83c057220410a..0a757d3368a44 100644 --- a/lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py +++ b/lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py @@ -79,16 +79,21 @@ def test(self): value = self.expect_expr("temp6", result_type="Foo<int *, int *>") self.assertFalse(value.GetType().GetTemplateArgumentValue(target, 1)) - # FIXME: support wider range of floating point types - value = self.expect_expr("temp7", result_type="Foo<__fp16, __fp16>") - self.assertFalse(value.GetType().GetTemplateArgumentValue(target, 1)) + value = self.expect_expr("temp7", result_type="Foo<__fp16, 1.000000e+00>") + template_param_value = value.GetType().GetTemplateArgumentValue(target, 1) + self.assertEqual(template_param_value.GetTypeName(), "__fp16") + # FIXME: returns incorrect value? + self.assertEqual(template_param_value.GetValueAsSigned(), 0) # The target we use when evaluating these expressions for Arm leads to there # not being a __bf16 type in the AST so we fall back to __fp16 and evaluating # this fails. if lldbplatformutil.getArchitecture() != "arm": - value = self.expect_expr("temp8", result_type="Foo<__bf16, __bf16>") - self.assertFalse(value.GetType().GetTemplateArgumentValue(target, 1)) + value = self.expect_expr("temp8", result_type="Foo<__bf16, 1.000000e+00>") + # FIXME: typename should be __bf16 + self.assertEqual(template_param_value.GetTypeName(), "__fp16") + # FIXME: returns incorrect value? + self.assertEqual(template_param_value.GetValueAsSigned(), 0) value = self.expect_expr("temp9", result_type="Bar<double, 1.200000e+00>") template_param_value = value.GetType().GetTemplateArgumentValue(target, 1) diff --git a/lldb/unittests/Symbol/TestTypeSystemClang.cpp b/lldb/unittests/Symbol/TestTypeSystemClang.cpp index ef13c788ee4a6..b88f14d2062f0 100644 --- a/lldb/unittests/Symbol/TestTypeSystemClang.cpp +++ b/lldb/unittests/Symbol/TestTypeSystemClang.cpp @@ -1360,15 +1360,44 @@ TEST_F(TestTypeSystemClang, TestIsRealFloatingPointType) { EXPECT_FALSE( m_ast->GetType(ast.getVectorType(ast.FloatTy, 1, VectorKind::Generic)) .IsRealFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.HalfTy).IsRealFloatingPointType()); EXPECT_TRUE(m_ast->GetType(ast.FloatTy).IsRealFloatingPointType()); EXPECT_TRUE(m_ast->GetType(ast.DoubleTy).IsRealFloatingPointType()); EXPECT_TRUE(m_ast->GetType(ast.LongDoubleTy).IsRealFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.Float128Ty).IsRealFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.BFloat16Ty).IsRealFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.Ibm128Ty).IsRealFloatingPointType()); +} + +TEST_F(TestTypeSystemClang, TestIsFloatingPointType) { + // Tests CompilerType::IsFloatingPointType + + const ASTContext &ast = m_ast->getASTContext(); - // FIXME: these should be true - EXPECT_FALSE(m_ast->GetType(ast.HalfTy).IsRealFloatingPointType()); - EXPECT_FALSE(m_ast->GetType(ast.Float128Ty).IsRealFloatingPointType()); - EXPECT_FALSE(m_ast->GetType(ast.BFloat16Ty).IsRealFloatingPointType()); - EXPECT_FALSE(m_ast->GetType(ast.Ibm128Ty).IsRealFloatingPointType()); + // Vectors of floats + EXPECT_FALSE( + m_ast->GetType(ast.getVectorType(ast.FloatTy, 1, VectorKind::Generic)) + .IsFloatingPointType()); + EXPECT_FALSE( + m_ast->GetType(ast.getVectorType(ast.DoubleTy, 1, VectorKind::Generic)) + .IsFloatingPointType()); + + // Complex floats + EXPECT_TRUE( + m_ast->GetType(ast.getComplexType(ast.FloatTy)).IsFloatingPointType()); + EXPECT_TRUE( + m_ast->GetType(ast.getComplexType(ast.DoubleTy)).IsFloatingPointType()); + EXPECT_FALSE( + m_ast->GetType(ast.getComplexType(ast.IntTy)).IsFloatingPointType()); + + // Builtin floats + EXPECT_TRUE(m_ast->GetType(ast.HalfTy).IsFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.FloatTy).IsFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.DoubleTy).IsFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.LongDoubleTy).IsFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.Float128Ty).IsFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.BFloat16Ty).IsFloatingPointType()); + EXPECT_TRUE(m_ast->GetType(ast.Ibm128Ty).IsFloatingPointType()); } TEST_F(TestTypeSystemClang, TestGetIsComplexType) { `````````` </details> https://github.com/llvm/llvm-project/pull/179213 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
