This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer.
ClosedPublic

Authored by ABataev on May 31 2017, 11:44 AM.

Details

Summary

If the first parameter of the function is the ImplicitParamDecl codegen
automatically marks it as an implicit argument with this or self
pointer. To fix this problem Implicit ThisOrSelfAttr is added. This
attribute is used to mark real this or self pointers only.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev created this revision.May 31 2017, 11:44 AM
aaron.ballman edited edge metadata.Jun 1 2017, 10:10 AM

Can you help me to understand what problem is being solved with this new attribute? Under what circumstances would the first argument be an ImplicitParamDecl but not an implicit this or self?

Can you help me to understand what problem is being solved with this new attribute? Under what circumstances would the first argument be an ImplicitParamDecl but not an implicit this or self?

For captured regions an outlined function is created, where all parameters are ImplicitParamDecls. And the very first parameter is wrongly treated as 'this' argument of the member function.

Can you help me to understand what problem is being solved with this new attribute? Under what circumstances would the first argument be an ImplicitParamDecl but not an implicit this or self?

For captured regions an outlined function is created, where all parameters are ImplicitParamDecls. And the very first parameter is wrongly treated as 'this' argument of the member function.

Ah, thank you! That makes sense to me, but it begs the question: why an attribute rather than a bit on ImplicitParamDecl?

rjmccall edited edge metadata.Jun 1 2017, 10:38 AM

Can you help me to understand what problem is being solved with this new attribute? Under what circumstances would the first argument be an ImplicitParamDecl but not an implicit this or self?

For captured regions an outlined function is created, where all parameters are ImplicitParamDecls. And the very first parameter is wrongly treated as 'this' argument of the member function.

Ah, thank you! That makes sense to me, but it begs the question: why an attribute rather than a bit on ImplicitParamDecl?

I agree: it would make more sense for ImplicitParamDecl to store a Kind that would always be provided at construction.

Can you help me to understand what problem is being solved with this new attribute? Under what circumstances would the first argument be an ImplicitParamDecl but not an implicit this or self?

For captured regions an outlined function is created, where all parameters are ImplicitParamDecls. And the very first parameter is wrongly treated as 'this' argument of the member function.

Ah, thank you! That makes sense to me, but it begs the question: why an attribute rather than a bit on ImplicitParamDecl?

I agree: it would make more sense for ImplicitParamDecl to store a Kind that would always be provided at construction.

Ok, will add a field to ImplicitParamDecl, thanks

ABataev updated this revision to Diff 101199.Jun 2 2017, 6:23 AM

Updates after review

rjmccall added inline comments.Jun 2 2017, 9:28 AM
include/clang/AST/Decl.h
1388 ↗(On Diff #101199)

Is there a good reason not to have a little enum here and require the caller to pass it down? There's only 20 different call sites, it shouldn't be hard to quickly classify them.

I think we should at least have:

ObjCSelf
ObjCCmd
CXXThis
CXXVTT

and then if you want to categorize everything else as Other, that's fine.

lib/CodeGen/CGDebugInfo.cpp
3466 ↗(On Diff #101199)

I don't think you need the ArgNo conditions anymore.

ABataev updated this revision to Diff 101262.Jun 2 2017, 12:34 PM

Added different kinds of ImplicitParamDecl.

ABataev updated this revision to Diff 101402.Jun 5 2017, 7:01 AM

Added DeclContext parameter to constructors of ImplicitParamDecl class.

rjmccall added inline comments.Jun 5 2017, 5:25 PM
include/clang/AST/Decl.h
1387 ↗(On Diff #101402)

I would just call this "Other" and document it as being for kinds of implicit parameters that we haven't seen a purpose in categorizing yet. (Or you could just classify them all, I suppose.)

We can use C++11 features in Clang now, so I would recommend hoisting this type out of ImplicitParamDecl and making it an enum class. You can then drop the "IPK_" prefixes.

1394 ↗(On Diff #101402)

Comment is out-of-date.

Also, please compress this into VarDecl's bit-field storage.

1405 ↗(On Diff #101402)

I see the point in having a convenience constructor like this, but it should probably still take an IPK.

1426 ↗(On Diff #101402)

Same idea. This should probably still take an IPK.

1429 ↗(On Diff #101402)

This comment is out-of-date.

lib/CodeGen/CGDebugInfo.cpp
3475 ↗(On Diff #101402)

Looks like this whole block is still unnecessarily indented.

lib/CodeGen/CGException.cpp
1654 ↗(On Diff #101402)

These do not feel like good uses of your shorthand constructors.

ABataev marked 7 inline comments as done.Jun 6 2017, 7:34 AM
ABataev added inline comments.
include/clang/AST/Decl.h
1387 ↗(On Diff #101402)

It's hard to classify them, in most cases they just represent some general parameters used for correct codegen of function types and that's it.

ABataev updated this revision to Diff 101571.Jun 6 2017, 8:31 AM
ABataev marked an inline comment as done.

Address John comments.

aaron.ballman added inline comments.Jun 7 2017, 7:43 AM
include/clang/AST/Decl.h
901 ↗(On Diff #101571)

It's a bit strange to me that the non-parameter declaration bits now have a field for implicit parameter information. Why here instead of ParmVarDeclBits?

1383 ↗(On Diff #101571)

Rather than use three access specifiers, can you reorder this?

class ... {
  void anchor() override;

public:
  ...
};
lib/CodeGen/CGDebugInfo.cpp
3471 ↗(On Diff #101571)

const auto * please.

3586 ↗(On Diff #101571)

const auto *

lib/CodeGen/CGOpenMPRuntime.cpp
3467 ↗(On Diff #101571)

This no longer sets the SourceLocation -- is that intended?

3479–3480 ↗(On Diff #101571)

This code would be cleaner if ImplicitParamDecl::Create() took a SourceLocation as it originally did. Perhaps the last param could be a default argument that defaults to SourceLocation{}?

lib/CodeGen/CGStmtOpenMP.cpp
284 ↗(On Diff #101571)

Why use a local variable here?

lib/CodeGen/ItaniumCXXABI.cpp
1411 ↗(On Diff #101571)

Can use auto * here.

lib/CodeGen/MicrosoftCXXABI.cpp
1416 ↗(On Diff #101571)

auto *

1429 ↗(On Diff #101571)

auto *

lib/Sema/SemaStmt.cpp
3959 ↗(On Diff #101571)

auto * (and elsewhere, I'll stop posting about them.)

lib/Serialization/ASTWriterDecl.cpp
918 ↗(On Diff #101571)

const auto *

ABataev added inline comments.Jun 7 2017, 7:48 AM
include/clang/AST/Decl.h
901 ↗(On Diff #101571)

Actually, ImplicitParamDecl already uses some bits from the NonParmVarDeclBitfields, at least it may be marked as ARCPseudoStrong for ObjC. That's why I had to reuse NonParmVarDeclBitfields part.

1383 ↗(On Diff #101571)

Ok

lib/CodeGen/CGOpenMPRuntime.cpp
3467 ↗(On Diff #101571)

Just missed this after some reworks, will return it back

aaron.ballman added inline comments.Jun 7 2017, 7:55 AM
include/clang/AST/Decl.h
901 ↗(On Diff #101571)

Ew. That's nasty and we should probably fix that (not as part of this patch). Can you add a FIXME here?

ABataev added inline comments.Jun 7 2017, 7:58 AM
include/clang/AST/Decl.h
901 ↗(On Diff #101571)

Ok, I will add FIXME for ARCPseudoStrong and for the ImplicitParamKind bitfields

ABataev updated this revision to Diff 101751.Jun 7 2017, 8:54 AM

Update after review

rjmccall added inline comments.Jun 7 2017, 3:35 PM
include/clang/AST/Decl.h
1387 ↗(On Diff #101402)

That's fair.

901 ↗(On Diff #101571)

The FIXME doesn't make sense because ImplicitParamDecl is not a subclass of ParamVarDecl.

The comment here needs to be updated.

ABataev updated this revision to Diff 101908.Jun 8 2017, 7:04 AM

Removed FIXMEs and corrected comment

aaron.ballman accepted this revision.Jun 8 2017, 1:56 PM

Aside from one minor nit, LGTM!

include/clang/AST/Decl.h
901 ↗(On Diff #101571)

The FIXME doesn't make sense because ImplicitParamDecl is not a subclass of ParamVarDecl.

Then the class name is poor, because I would not expect to find *anything* about parameter declarations (implicit or otherwise) in a class named NonParmVarDeclBitfields. However, I see the logic behind the split better now, so thank you for that. (Nothing needs to be done here for this patch.)

lib/Serialization/ASTWriterDecl.cpp
918 ↗(On Diff #101571)

This is still missing the const qualifier.

This revision is now accepted and ready to land.Jun 8 2017, 1:56 PM
rjmccall accepted this revision.Jun 8 2017, 4:25 PM
rjmccall added inline comments.
include/clang/AST/Decl.h
901 ↗(On Diff #101571)

Yeah, I agree that it's unfortunate that we have two separate classes for parameter declarations.

This revision was automatically updated to reflect the committed changes.