Set a CXXRecordDecl to not be passed in registers if DW_CC_pass_by_reference when loading from DWARF

[shafik]

Summary

This will fix a bug where during expression parsing we are not setting a CXXRecordDecl to not be passed in registers and the resulting code generation is wrong.

The DWARF attribute DW_CC_pass_by_reference tells us that we should not be passing in registers i.e. RAA_Indirect.

This change depends this clang change which fixes the fact that the ASTImporter does not copy RecordDeclBits for CXXRecordDecl: https://reviews.llvm.org/D61140

[shafik]

@rsmith I tagged you in this change in case we are missing any implications in using DW_CC_pass_by_reference to do setArgPassingRestrictions(clang::RecordDecl::APK_CannotPassInRegs);

[teemperor]

packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/main.cpp

40 ↗

(On Diff #196716)

Some small things:

1. I think the source here is not clang-formatted :)
2. It's not really clear to me if Shape or Bounds are supposed to have arg passing restrictions (or both?). Maybe rename them or comment this in the source. E.g. // supposed to be passed by ref/value.
3. Can this test be more minimized? Do we need both x and y as member variables? Do we need all these methods and variables? Especially when debug stepping through this test case at some point a minimized test case is always nicer.

[aprantl]

packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/TestArgumentPassingRestrictions.py

26 ↗

(On Diff #196716)

What does False do if it isn't the default argument? And if it is, can we remove it?

29 ↗

(On Diff #196716)

it's probably more future-proof to write this as substrs=['(Bounds)', 'x = 1', 'y = 2'])

packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/main.cpp

1 ↗

(On Diff #196716)

Any comment at all to explain what's special here?

11 ↗

(On Diff #196716)

clang-format please

22 ↗

(On Diff #196716)

what's the point of the return?

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

965 ↗

(On Diff #196716)

at least in LLVM style we elide single-statement braces.

967 ↗

(On Diff #196716)

This part LGTM.

[shafik]

Changed to reflect comments.

• Added comments to test to explain what it is doing.
• Formatting and other minor fixes.

packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/main.cpp

22 ↗

(On Diff #196716)

This is vestigial return left over from when the reproducer was more complex.

40 ↗

(On Diff #196716)

I am adding comments to the code to try to address what it is trying to catch.

I have tried to simplify as much as possible but the scenario is rather elaborate.

I don't need both x and y though.

[shafik]

@aprantl @teemperor I believe I addressed your comments.

[friss]

Correct me if I'm wrong, but the test seems overly complicated still. We are testing that we can get the return value of a type that needs to be passed by reference. Calling simple free function Bounds bounds() should trigger this code path, shouldn't it? I would be interested to know wether we do the right thing when passing such a struct by value in the source code too as Clang doesn't seem to differentiate return values and arguments. Something like:

// This structure has a non-trivial copy constructor so
// it needs to be passed by reference.
struct PassByRef {
  PassByRef() = default;
  PassByRef(const PassByRef &p);

  int x = 11223344;
};

PassByRef returnPassByRef() { return PassByRef(); }
int takePassByRef(PassByRef p) {
    return p.x;
}

int main() {
    PassByRef p = returnPassByRef();
    p.x = 42;
    return takePassByRef(returnPassByRef())' // Break here
}

Break on the return and evaluate returnPassByRef() and takePassByRef(p).

[teemperor]

As we're anyway throwing in reduced test cases, here is another version:

struct PassByRef {
  // PassByRef should be pass by reference since the destructor is
  // user-defined which means it can not be passed in registers.
  ~PassByRef() {}
  int x = 0;
};

struct Foo {
  PassByRef bar(Foo *f) const {
    PassByRef b;
    if (this == f)
      b.x = 11223344;
    return b;
  }
};

int main() {
  Foo f;
  return f.bar(&f).x; // break here
}

We can save the typing work for the two constructors by just making a non-trivial destructor.

(Also this patch currently deletes TestGlobalVariables.py which seems unintentional)

[shafik]

• Simplifying test
• Fixing unintended deleted test

[shafik]

@teemperor good call, that is indeed simpler and yes I did not intend that delete.

[friss]

packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/TestArgumentPassingRestrictions.py

28–29 ↗

(On Diff #197618)

This still tests only getting the return value and not passing the struct as an argument.

[shafik]

Testing both passing as and argument and returning

[shafik]

@friss added second test

[friss]

packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/TestArgumentPassingRestrictions.py

32 ↗

(On Diff #197628)

If this test passes, we have a problem. The code modifies p.x to be 42, so that what it should return.

[shafik]

Modifying copy contructor to be act more intuitively.

packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/TestArgumentPassingRestrictions.py

32 ↗

(On Diff #197628)

The copy constructor does not actually copy and we are default constructing since it is pass by value, but that is probably too clever so I changed it.

[friss]

Thanks, indeed this looks better.

lldb/trunk/packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/Makefile

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/TestArgumentPassingRestrictions.py

+"""
+This is a test to ensure that both lldb is reconstructing the right
+calling convention for a CXXRecordDecl as represented by:
+
+   DW_CC_pass_by_reference
+   DW_CC_pass_by_value
+
+and to also make sure that the ASTImporter is copying over this
+setting when importing the CXXRecordDecl via setArgPassingRestrictions.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestArgumentPassingRestrictions(TestBase):
+
+  mydir = TestBase.compute_mydir(__file__)
+
+  def test_argument_passing_restrictions(self):
+    self.build()
+
+    lldbutil.run_to_source_breakpoint(self, '// break here',
+            lldb.SBFileSpec("main.cpp"))
+
+    self.expect("expr returnPassByRef()",
+            substrs=['(PassByRef)', '= 11223344'])
+
+    self.expect("expr takePassByRef(p)",
+            substrs=['(int)', '= 42'])

lldb/trunk/packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/main.cpp

+// This structure has a non-trivial copy constructor so
+// it needs to be passed by reference.
+struct PassByRef {
+  PassByRef() = default;
+  PassByRef(const PassByRef &p){x = p.x;};
+
+  int x = 11223344;
+};
+
+PassByRef returnPassByRef() { return PassByRef(); }
+int takePassByRef(PassByRef p) {
+    return p.x;
+}
+
+int main() {
+    PassByRef p = returnPassByRef();
+    p.x = 42;
+    return takePassByRef(p); // break here
+}

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (type_ptr == NULL) {
         if (calling_convention == llvm::dwarf::DW_CC_pass_by_value) {
           clang::CXXRecordDecl *record_decl =
                   m_ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType());
           if (record_decl) {
             record_decl->setHasTrivialSpecialMemberForCall();
           }
         }
 
+        if (calling_convention == llvm::dwarf::DW_CC_pass_by_reference) {
+          clang::CXXRecordDecl *record_decl =
+              m_ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType());
+          if (record_decl)
+            record_decl->setArgPassingRestrictions(
+                clang::RecordDecl::APK_CannotPassInRegs);
+        }
+
       } break;
 
       case DW_TAG_enumeration_type: {
         // Set a bit that lets us know that we are currently parsing this
         dwarf->GetDIEToType()[die.GetDIE()] = DIE_IS_BEING_PARSED;
 
         bool is_scoped = false;
         DWARFFormValue encoding_form;