Page MenuHomePhabricator

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

Authored by shafik on Apr 25 2019, 1:41 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

shafik created this revision.Apr 25 2019, 1:41 PM
shafik edited the summary of this revision. (Show Details)

@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 added inline comments.Apr 29 2019, 3:23 PM
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 added inline comments.Apr 29 2019, 3:30 PM
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 updated this revision to Diff 197426.Apr 30 2019, 1:23 PM
shafik marked 9 inline comments as done.

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 added a comment.May 1 2019, 8:57 AM

@aprantl @teemperor I believe I addressed your comments.

friss added a subscriber: friss.May 1 2019, 10:23 AM

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).

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 updated this revision to Diff 197618.May 1 2019, 12:52 PM
  • Simplifying test
  • Fixing unintended deleted test

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

friss added inline comments.May 1 2019, 1:07 PM
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 updated this revision to Diff 197628.May 1 2019, 1:30 PM

Testing both passing as and argument and returning

shafik added a comment.May 1 2019, 1:30 PM

@friss added second test

friss added inline comments.May 1 2019, 1:58 PM
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 updated this revision to Diff 197636.May 1 2019, 2:05 PM
shafik marked an inline comment as done.

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.

shafik marked an inline comment as done.May 1 2019, 2:05 PM
friss accepted this revision.May 1 2019, 2:33 PM

Thanks, indeed this looks better.

This revision is now accepted and ready to land.May 1 2019, 2:33 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2019, 3:22 PM