[Lldb-commits] [lldb] r351250 - Simplify Value::GetValueByteSize()

[Pavel Labath via lldb-commits]

Hi Adrian, Zachary,

this commit breaks the SymbolFile/NativePDB/function-types-builtins.cpp 
test 
<http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/630>. 
This is due do a slight behavioral change where previously this function 
would consider a value 0 from GetByteSize to be failure, but now it's 
considered a success. This seemed to matter for function references, 
where it changed the output when displaying them (though I can't really 
say whether it's for the better or worse).

I am not sure whether this change was intentional or not (it would seem 
to be in line with the overall direction you're going in with the 
Optional patches, but the commit message just says "simplify"), and I am 
also not sure whether the that test intentionally meant to check this 
error message (it seems somewhat irrelevant to that test, but maybe 
Zachary put it there for some reason). For the time being, I've reverted 
the patch to keep the build green.

BTW: You should be able to reproduce this failure if you add lld to your 
build config. Alternatively you can just compile that source file for 
darwin, as the same behavioral change is present with dwarf debug info too.


On 15/01/2019 22:26, Adrian Prantl via lldb-commits wrote:
> Author: adrian
> Date: Tue Jan 15 13:26:03 2019
> New Revision: 351250
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=351250&view=rev
> Log:
> Simplify Value::GetValueByteSize()
> 
> Modified:
>      lldb/trunk/source/Core/Value.cpp
> 
> Modified: lldb/trunk/source/Core/Value.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Value.cpp?rev=351250&r1=351249&r2=351250&view=diff
> ==============================================================================
> --- lldb/trunk/source/Core/Value.cpp (original)
> +++ lldb/trunk/source/Core/Value.cpp Tue Jan 15 13:26:03 2019
> @@ -210,35 +210,31 @@ bool Value::ValueOf(ExecutionContext *ex
>   }
>   
>   uint64_t Value::GetValueByteSize(Status *error_ptr, ExecutionContext *exe_ctx) {
> -  uint64_t byte_size = 0;
> -
>     switch (m_context_type) {
>     case eContextTypeRegisterInfo: // RegisterInfo *
> -    if (GetRegisterInfo())
> -      byte_size = GetRegisterInfo()->byte_size;
> +    if (GetRegisterInfo()) {
> +      if (error_ptr)
> +        error_ptr->Clear();
> +      return GetRegisterInfo()->byte_size;
> +    }
>       break;
>   
>     case eContextTypeInvalid:
>     case eContextTypeLLDBType: // Type *
>     case eContextTypeVariable: // Variable *
>     {
> -    const CompilerType &ast_type = GetCompilerType();
> -    if (ast_type.IsValid())
> -      if (llvm::Optional<uint64_t> size = ast_type.GetByteSize(
> -              exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr))
> -        byte_size = *size;
> -  } break;
> -  }
> -
> -  if (error_ptr) {
> -    if (byte_size == 0) {
> -      if (error_ptr->Success())
> -        error_ptr->SetErrorString("Unable to determine byte size.");
> -    } else {
> -      error_ptr->Clear();
> +    auto *scope = exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr;
> +    if (llvm::Optional<uint64_t> size = GetCompilerType().GetByteSize(scope)) {
> +      if (error_ptr)
> +        error_ptr->Clear();
> +      return *size;
>       }
> +    break;
> +  }
>     }
> -  return byte_size;
> +  if (error_ptr && error_ptr->Success())
> +    error_ptr->SetErrorString("Unable to determine byte size.");
> +  return 0;
>   }
>   
>   const CompilerType &Value::GetCompilerType() {
> 
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>