Page MenuHomePhabricator

[dwarf5] Support DebugInfo for constexpr for C++ variables and functions
Needs ReviewPublic

Authored by awpandey on Jan 23 2020, 2:50 AM.

Details

Summary

This patch adds dw_at_const_expr flag for the constant variable or functions in C++

Diff Detail

Event Timeline

awpandey created this revision.Jan 23 2020, 2:50 AM

I put in a lot of comments about spelling for the new parameter (constExpr, isConstexpr, isConstExpr) which should be named consistently throughout. Please do not use Constant or any variant, as that tends to mean something else.

But, what I would rather see instead of a new parameter everywhere: define a new DIFlag bit. Most of the churn goes away, and there's no bitcode compatibility issue. It's true that there are not many DIFlag bits left, but as this attribute can apply to both variables and functions, it seems appropriate to put it there.

llvm/include/llvm/IR/DIBuilder.h
585

\param isConstExpr
also wheather -> whether

668

Add \param constExpr here.

704–706

Add \param isConstExpr here

llvm/include/llvm/IR/DebugInfoMetadata.h
1694

todo?

2292

isConstExpr

2634–2636

IsConstExpr as this flag is specifically tied to the const_expr attribute; "constant" means something different.

2665

TODO?

2674

IsConstExpr

2682

IsConstExpr

2777–2779

IsConstExpr

2788–2789

IsConstExpr

2798–2799

IsConstExpr

2812–2814

IsConstExpr

2818

IsConstExpr

llvm/lib/AsmParser/LLParser.cpp
4679–4680

Document the new parameter.

4701

Naming convention wants this to be constExpr

4878

Document the new parameter.

4892

constExpr

4910

Document the new parameter.

4921

constExpr

llvm/lib/IR/DIBuilder.cpp
646

isConstExpr (throughout this module)

718

The comment should match the parameter name.

llvm/lib/IR/DebugInfo.cpp
468–469

todo?

479

todo?

836

todo?

llvm/lib/IR/DebugInfoMetadata.cpp
634–637

isConstExpr throughout this module.

llvm/lib/IR/LLVMContextImpl.h
892

isConstExpr

925

TODO?

969

commented-out bits of code?

awpandey updated this revision to Diff 240122.Jan 24 2020, 1:56 AM
awpandey marked 29 inline comments as done.

Hi @probinson, I have changed the patch according to your comments.

Is it necessary to use DIFlags? I am willing to do that but generally, it is not welcomed because we have a limited number of DIFlags and most of them are currently in use.

Is it necessary to use DIFlags? I am willing to do that but generally, it is not welcomed because we have a limited number of DIFlags and most of them are currently in use.

Are there any flags within DIFlags that could be applied only to functions? If so, we should move them in to the DISPFlags in order to make some space within the DIFlags.
In addition, does it make sense to making something like DIVarFlags? And move the flags applying only to variables.

Is it necessary to use DIFlags? I am willing to do that but generally, it is not welcomed because we have a limited number of DIFlags and most of them are currently in use.

Are there any flags within DIFlags that could be applied only to functions? If so, we should move them in to the DISPFlags in order to make some space within the DIFlags.

There might be. The trick is backward compatibility of bitcode. It looks like Virtual is already replicated to DISPFlags, but wasn't removed from DIFlags. Other candidates for migration to DISPFlags: Explicit, Prototyped, NoReturn, Thunk, AllCallsDescribed, maybe NonTrivial.
Their exact semantics and how they're used would have to be verified.

@dblaikie @aprantl thoughts on this?

huh. DIFlag bit 21 has no symbol; it used to be DIFlagMainSubprogram but we did not put in a Reserved or Deprecated name for it. That should be fixed.

In addition, does it make sense to making something like DIVarFlags? And move the flags applying only to variables.

We might have some like that. BitField and StaticMember, just based on the names.

Note that moving flags around is tricky and requires extreme care to ensure full backward compatibility. Moving flags would have to be its own patch and this one would depend on it.

I agree, we must pay attention and address the bitcode compatibility.

Thanks!

define a new DIFlag bit

That seems to be a good idea. Also please make sure to add a bitcode roundtrip test to test/Assembler/

Is it necessary to use DIFlags? I am willing to do that but generally, it is not welcomed because we have a limited number of DIFlags and most of them are currently in use.

Are there any flags within DIFlags that could be applied only to functions? If so, we should move them in to the DISPFlags in order to make some space within the DIFlags.

There might be. The trick is backward compatibility of bitcode. It looks like Virtual is already replicated to DISPFlags, but wasn't removed from DIFlags. Other candidates for migration to DISPFlags: Explicit, Prototyped, NoReturn, Thunk, AllCallsDescribed, maybe NonTrivial.
Their exact semantics and how they're used would have to be verified.

@dblaikie @aprantl thoughts on this?

Don't have strong feelings/knowledge here (Apple folks have more vested interested in the IR backwards compatibility issues). We can rev the Debug Info Version metadata to allow us to recycle those bits rather than leaving them dead/reserved/deprecated. Once we actually need bits, I expect we should do that - but sounds like we're not there quite yet.

huh. DIFlag bit 21 has no symbol; it used to be DIFlagMainSubprogram but we did not put in a Reserved or Deprecated name for it. That should be fixed.

In addition, does it make sense to making something like DIVarFlags? And move the flags applying only to variables.

We might have some like that. BitField and StaticMember, just based on the names.

Note that moving flags around is tricky and requires extreme care to ensure full backward compatibility. Moving flags would have to be its own patch and this one would depend on it.

General question: What purpose do you (or anyone else - @probinson, @aprantl) have for this attribute? I'd like to encourage the idea that we don't need to implement every feature DWARF provides if there's no planned use for it - things can/should be implemented as-motivated so we don't end up with weird features/implementations/etc that are essentially untested/unused.

clang/lib/CodeGen/CGDebugInfo.cpp
3406

Any reason isConstexpr shouldn't be specified here too?

clang/test/CodeGenCXX/constExpr.cpp
9

do these {{.*}} on the end of the lines provide any particular value?

Is it necessary to use DIFlags? I am willing to do that but generally, it is not welcomed because we have a limited number of DIFlags and most of them are currently in use.

Are there any flags within DIFlags that could be applied only to functions? If so, we should move them in to the DISPFlags in order to make some space within the DIFlags.

There might be. The trick is backward compatibility of bitcode. It looks like Virtual is already replicated to DISPFlags, but wasn't removed from DIFlags. Other candidates for migration to DISPFlags: Explicit, Prototyped, NoReturn, Thunk, AllCallsDescribed, maybe NonTrivial.
Their exact semantics and how they're used would have to be verified.

@dblaikie @aprantl thoughts on this?

huh. DIFlag bit 21 has no symbol; it used to be DIFlagMainSubprogram but we did not put in a Reserved or Deprecated name for it. That should be fixed.

In addition, does it make sense to making something like DIVarFlags? And move the flags applying only to variables.

We might have some like that. BitField and StaticMember, just based on the names.

Note that moving flags around is tricky and requires extreme care to ensure full backward compatibility. Moving flags would have to be its own patch and this one would depend on it.

One clean way of doing this is to reserve one previously unused bit in DIFlags or DISPFlags to indicate that these flags were moved over and teach MetadataLoader to decode the old and new format accordingly. In that case we'd want a spearate patch that just moves the flag, though, all flags should be moved in the same patch.

awpandey updated this revision to Diff 243509.Feb 10 2020, 4:18 AM

@probinson I have reimplemented the feature by using DIFlags.

djtodoro added inline comments.Feb 10 2020, 11:54 PM
llvm/include/llvm/IR/DebugInfoFlags.def
61 ↗(On Diff #243509)

We are almost out of space here... I wouldn't add any additional flag here before we do necessary refactoring.

As we discussed, it makes sense moving the function-related flags into the existing DISPFlags. In addition, we may want to introduce something like DIVarFlags, and move the variable-related flags into that.

Also, we need to pay attention on the bitcode compatibility as well. Back then, I have implemented something similar here: https://reviews.llvm.org/D59288, so it can be used as a reference.

I think we have to do it first, as a preparation patch for this one.

djtodoro added a comment.EditedFeb 11 2020, 12:03 AM

@awpandey Thanks for doing this, but could you please explain the motivation of implementing this? Can we use it (somehow) to generate more variables with the DW_AT_const_value?

@awpandey Thanks for doing this, but could you please explain the motivation of implementing this?

+1 (as I asked earlier & don't think there's been a response for)

Chirag added a subscriber: Chirag.Feb 17 2020, 4:12 AM