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.

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
1404–1406

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
3470

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
1392

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.

1399

Comment is out-of-date.

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

1407

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

1417

Same idea. This should probably still take an IPK.

1420

This comment is out-of-date.

lib/CodeGen/CGDebugInfo.cpp
3475

Looks like this whole block is still unnecessarily indented.

lib/CodeGen/CGException.cpp
1657

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
1392

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

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

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

class ... {
  void anchor() override;

public:
  ...
};
lib/CodeGen/CGDebugInfo.cpp
3471

const auto * please.

3586

const auto *

lib/CodeGen/CGOpenMPRuntime.cpp
3467

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

3479–3480

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

Why use a local variable here?

lib/CodeGen/ItaniumCXXABI.cpp
1411

Can use auto * here.

lib/CodeGen/MicrosoftCXXABI.cpp
1416

auto *

1429

auto *

lib/Sema/SemaStmt.cpp
3959

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

lib/Serialization/ASTWriterDecl.cpp
918

const auto *

ABataev added inline comments.Jun 7 2017, 7:48 AM
include/clang/AST/Decl.h
901

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

Ok

lib/CodeGen/CGOpenMPRuntime.cpp
3467

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

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

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
901

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

The comment here needs to be updated.

1392

That's fair.

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

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

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

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.