Remove bypass of GetAddressOf when dealing with dynamic class types in variable materialization

[augusto2112]

@adrian-prantl @vedantk

[adrian-prantl]

Q: Is m_is_generic dead code after this?

[augusto2112]

I left scalar_is_load_address = m_is_generic since I though the changes should be independent, but after removing that m_is_generic is indeed dead code!

[adrian-prantl]

@swift-ci test

[adrian-prantl]

Just to clarify: This brings the code in sync with llvm.org again?

[adrian-prantl]

This is great. The fewer swift-lldb-specific changes we need, the better!

[augusto2112]

Just to clarify: This brings the code in sync with llvm.org again?

@adrian-prantl I didn't include the change of scalar_is_load_address in this PR (should I add that here?), but the changes of this and #1452 would leave the definition of scalar_is_load_address and addr_of_valobj the same as in llvm.org.

I'm not sure if you're asking if the code following that is the same as well, so here are the differences I found for the rest of the function:

• There is a Swift-exclusive block of code:

      // BEGIN Swift.
      if (lldb::ProcessSP process_sp =
          map.GetBestExecutionContextScope()->CalculateProcess())
        if (auto runtime = process_sp->GetLanguageRuntime(
                valobj_type.GetMinimumLanguage())) {
          Status read_error;
          addr_of_valobj =
              runtime->FixupAddress(addr_of_valobj, valobj_type, read_error);
        }
      // END Swift.

• There's also a Swift only check for empty structs when materialization fails:

          if (valobj_type.GetMinimumLanguage() == lldb::eLanguageTypeSwift) {
            llvm::Optional<uint64_t> size =
                valobj_type.GetByteSize(frame_sp.get());
            if (size && *size == 0) {
              // We don't need to materialize empty structs in Swift.
              return;
            }
          }

• GetByteSize call is different:
  • Swift: if (data.GetByteSize() < m_variable_sp->GetType()->GetByteSize(scope))
  • LLVM: if (data.GetByteSize() < m_variable_sp->GetType()->GetByteSize())
• Another call to GetByteSize:
  • Swift: m_variable_sp->GetType()->GetByteSize(map.GetBestExecutionContextScope()) .getValueOr(0)
  • LLVM: m_variable_sp->GetType()->GetByteSize().getValueOr(0)

[adrian-prantl]

Just to clarify: This brings the code in sync with llvm.org again?

@adrian-prantl I didn't include the change of scalar_is_load_address in this PR (should I add that here?), but the changes of this and #1452 would leave the definition of scalar_is_load_address and addr_of_valobj the same as in llvm.org.

I'm not sure if you're asking if the code following that is the same as well, so here are the differences I found for the rest of the function:

• There is a Swift-exclusive block of code:

      // BEGIN Swift.
      if (lldb::ProcessSP process_sp =
          map.GetBestExecutionContextScope()->CalculateProcess())
        if (auto runtime = process_sp->GetLanguageRuntime(
                valobj_type.GetMinimumLanguage())) {
          Status read_error;
          addr_of_valobj =
              runtime->FixupAddress(addr_of_valobj, valobj_type, read_error);
        }
      // END Swift.

I think this one is strictly necessary. IIRC, it is to support global resilient values.

• There's also a Swift only check for empty structs when materialization fails:

          if (valobj_type.GetMinimumLanguage() == lldb::eLanguageTypeSwift) {
            llvm::Optional<uint64_t> size =
                valobj_type.GetByteSize(frame_sp.get());
            if (size && *size == 0) {
              // We don't need to materialize empty structs in Swift.
              return;
            }
          }

I added support for zero-sized types to llvm.org's lldb ~1 year ago, maybe this is no longer necessary, or could be upstreamed.

• GetByteSize call is different:

  • Swift: if (data.GetByteSize() < m_variable_sp->GetType()->GetByteSize(scope))
  • LLVM: if (data.GetByteSize() < m_variable_sp->GetType()->GetByteSize())

• Another call to GetByteSize:

  • Swift: m_variable_sp->GetType()->GetByteSize(map.GetBestExecutionContextScope()) .getValueOr(0)
  • LLVM: m_variable_sp->GetType()->GetByteSize().getValueOr(0)

That is also necessary. In many cases we need to ask GetByteSize() to the Swift runtime, and we can only do that with an ExecutionContext. We should eventually upstream this change.

Thanks for the analysis!