This is an archive of the discontinued LLVM Phabricator instance.

DebugInfo: use strongly typed enum for debug info flags
ClosedPublic

Authored by vleschuk on Aug 22 2016, 6:42 AM.

Details

Summary

Use ADT/BitmaskEnum for DINode::DIFlags for the following purposes:

  • Get rid of unsigned int for flags to avoid problems on platforms with sizeof(int) < 4
  • Flags are now strongly typed

Diff Detail

Repository
rL LLVM

Event Timeline

vleschuk updated this revision to Diff 68852.Aug 22 2016, 6:42 AM
vleschuk retitled this revision from to DebugInfo: introduce di_flags_t type for debug info flags.
vleschuk updated this object.
vleschuk added reviewers: dexonsmith, aprantl, echristo.
vleschuk added a subscriber: llvm-commits.
aprantl edited edge metadata.Aug 22 2016, 8:51 AM

Having a separate type for the flags could help in making the DIBuilder interfaces (where we often have 3+ integer parameters in a row) more typesafe.

Note that the spelling doesn't conform to the LLVM style: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

majnemer added inline comments.
include/llvm/IR/DebugInfoMetadata.h
49 ↗(On Diff #68852)

Could we just declare enum DIFlags : uint32_t in DIBuilder.h and use that in the DIBuilder interface instead of introducing a new type name?

echristo added inline comments.Aug 22 2016, 10:23 AM
include/llvm/IR/DebugInfoMetadata.h
49 ↗(On Diff #68852)

+1

vleschuk added inline comments.Aug 25 2016, 12:02 PM
include/llvm/IR/DebugInfoMetadata.h
49 ↗(On Diff #68852)

If we do that we still need some typedef, otherwise there still can be problems. Imagine code like this:

enum DIFlags: uint32_t { flag1 = (1 << 1), ... , (1 << 17) };

void foo(??? Flags /* some integral constant containing flags combination */);

What should we place instead of "???"? If we use uint32_t explicitly, we lose the abstraction and ability to change the underlying type of enum just in one place in future. We can use std::underlying_type<> but this is too ugly or we need to typedef it. That's why I think we still need to add a new typedef.

vleschuk updated this revision to Diff 69331.Aug 26 2016, 2:34 AM
vleschuk retitled this revision from DebugInfo: introduce di_flags_t type for debug info flags to DebugInfo: introduce DIFlagsUnderlying type for debug info flags.
vleschuk updated this object.
vleschuk edited edge metadata.

Changed type name according to naming conventions.

Changed underlying flags type name to match naming requirements. I think having additional typedef here is necessary that's the idea of the patch (also described in inline comment). As for moving DIFlags to DIBuilder: I don't think this is a good idea: this type logically belongs to DebugInfoMetadata (which is used by DIBuilder). Declaring DIFlags in DIBuilder will lead to cross-dependency.

Can somebody please look at it please? Are there any comments/objections to my recent replies?
Thanks in advance.

dblaikie edited edge metadata.Aug 29 2016, 11:17 AM

Why is it that we aren't just using teh DIFlags type directly everywhere we are currently using unsigned (everywhere you're changing to DIFlagsUnderlying)?

Why is it that we aren't just using teh DIFlags type directly everywhere we are currently using unsigned (everywhere you're changing to DIFlagsUnderlying)?

Because constructions like the following do not compile

enum DIFlags: uint32_t
{
    Flag1 = 1,
    Flag2 = 1 << 2,
    Flag3 = 1 << 3,
};

// .... 

DIFlags Flags = Flag1 | Flag2; // expression (Flag1 | Flag2) isn't DIFlags, it results in underlying type
clang++ -std=c++11 t.cpp -o t
t.cpp:12:13: error: cannot initialize a variable of type 'DIFlags' with an rvalue of type 'unsigned int'
    DIFlags Flags = Flag1 | Flag2;
            ^       ~~~~~~~~~~~~~
1 error generated.
vleschuk updated this revision to Diff 69753.Aug 30 2016, 2:13 PM
vleschuk retitled this revision from DebugInfo: introduce DIFlagsUnderlying type for debug info flags to DebugInfo: use strongly typed enum for debug info flags.
vleschuk updated this object.
vleschuk edited edge metadata.
  • Switched to ADT/BitmaskEnum for DIFlags
  • Added "special" flags: FlagLargest and FlagZero (FlagLargest can be disabled in DebugInfoFlags.def to avoid problems with duplicate cases in switch statements)
  • For types derived from DINode added several overloaded methods which take either underlying enum type or enum itself. This was done to avoid lots of static_casts in other modules.
  • Edited tests to use DIFlags
dblaikie added inline comments.Aug 30 2016, 2:16 PM
include/llvm/IR/DebugInfoMetadata.h
175–182 ↗(On Diff #69753)

Should we move this outside the class so it doesn't have to be qualified in lots of places? The name already has the 'scoping' of the DI prefix, not sure it's particularly helpful that it's inside DINode too.

183 ↗(On Diff #69753)

I'm assuming we don't need DIFlagsUnderlying anymore? Can we just use DIFlags everywhere?

Ah, I guess having DIFlags inside DINode helps scope the enumerators inside it (DINode::FlagZero rather than having "FlagZero" pollute the llvm namespace).

vleschuk added inline comments.Aug 30 2016, 2:24 PM
include/llvm/IR/DebugInfoMetadata.h
183 ↗(On Diff #69753)

Nope. For example see updated ::get definitions in classes derived from DINode:

DEFINE_MDNODE_GET(DIDerivedType,
                    (unsigned Tag, MDString *Name, Metadata *File,
                     unsigned Line, Metadata *Scope, Metadata *BaseType,
                     uint64_t SizeInBits, uint64_t AlignInBits,
                     uint64_t OffsetInBits, **DIFlagsUnderlying** Flags,
                     Metadata *ExtraData = nullptr),
                    (Tag, Name, File, Line, Scope, BaseType, SizeInBits,
                     AlignInBits, OffsetInBits, **static_cast<DIFlags>(Flags)**, ExtraData))

This helps to avoid lots of static_casts in client code. Also some utility functions like splitFlags require this underlying type to process bitmasks which can consist not only of enumerated flags.

In other words, this typedef is used in order not to break old code and avoid lots of code rewriting.

Ah, I guess having DIFlags inside DINode helps scope the enumerators inside it (DINode::FlagZero rather than having "FlagZero" pollute the llvm namespace).

Correct. I also wanted to move DIFlags outside DINode, but than reconsidered it.

vleschuk updated this revision to Diff 69802.Aug 31 2016, 12:42 AM

Got rid of DIFlagsUnderlying typedef, adopted DebugInfoMetadata users and relevant tests to use DIFlags enum.

dblaikie added inline comments.Aug 31 2016, 4:09 PM
include/llvm/IR/DebugInfoFlags.def
44 ↗(On Diff #69802)

The double negative seems a bit confusing and this is used in exactly one place - so perhaps it should be opt-in, rather than opt-out. (eg: "ifdef DI_FLAG_LARGEST" and then just define it in the one place it's needed, rather than having to define NO_DI_FLAG_LARGEST in several places)

vleschuk updated this revision to Diff 69946.Aug 31 2016, 11:07 PM

Switched NO_DI_FLAG_LARGEST -> DI_FLAG_LARGEST_NEEDED macro to avoid double negation (e.g. #ifndef NO_DI_FLAG_LARGEST). For now defining DI_FLAG_LARGEST_NEEDED is required only in one place (defining previous macro was required in 3 places).

vleschuk marked an inline comment as done.Aug 31 2016, 11:08 PM
vleschuk added inline comments.
include/llvm/IR/DebugInfoFlags.def
44 ↗(On Diff #69946)

Done, see new diff.

vleschuk updated this revision to Diff 70056.Sep 1 2016, 12:59 PM
vleschuk marked an inline comment as done.

Removed unnecessary assert().

dblaikie accepted this revision.Sep 1 2016, 2:35 PM
dblaikie edited edge metadata.
This revision is now accepted and ready to land.Sep 1 2016, 2:35 PM

Thanks everybody! Could someone commit this please (along with the dependency https://reviews.llvm.org/D23767 for clang)? I don't have commit rights yet.

Thanks in advance!

vleschuk updated this revision to Diff 70135.Sep 1 2016, 11:51 PM
vleschuk edited edge metadata.

Removed few deprecated tests which don't make sense with new type for flags.

vleschuk added a comment.EditedSep 3 2016, 2:53 PM

The conversation doesn't seem to appear here. So I repost (with few more details):

Removed few deprecated tests which don't make sense with new type for flags.

@dblaikie wrote:

Are those tests actually invalid now? You can still stuff non-flag values into the flag with a static cast, so perhaps we should keep the test around in that form?

The removed tests trigger assert() in BitmaskEnum. So these tests look invalid for me now.

/// Check that Val is in range for E, and return Val cast to E's underlying     
/// type.                                                                       
template <typename E> typename std::underlying_type<E>::type Underlying(E Val) {
  auto U = static_cast<typename std::underlying_type<E>::type>(Val);            
  assert(U >= 0 && "Negative enum values are not allowed.");                    
  assert(U <= Mask<E>() && "Enum value too large (or largest val too small?)");   // <===== this one
  return U;                                                                     
}

Please let me know if you have any objections.

Hello all, if there are no objects, could someone commit this for me please? I don't have commit rights yet.

Thanks in advance.

(Ran git clang-format and fixed a warning before committing)

This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Sep 5 2016, 8:35 PM

I am looking at the first failure right now. As for the other 2: they are caused by the fact that build was started before the dependent patch for clang was committed: https://reviews.llvm.org/D23767 <- this one (already accepted but not committed) makes clang use llvm::DINode::DIFlags instead of int.

(Ran git clang-format and fixed a warning before committing)

	​  default:
	​    return "";

The test failed because this change removed "default" case from getFlagsString() function.

vleschuk updated this revision to Diff 70367.Sep 6 2016, 2:52 AM
vleschuk removed rL LLVM as the repository for this revision.

Got rid of the following warning:

warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

in getFlagString() function. Removed "default" from switch/case, put " return ""; " outside switch construction.

The default case is required in this function in case someone puts a combination of Flags (or garbage) into it. These cases are covered in

unittests/IR/DebugInfoTest.cpp - they were the tests that failed after @mehdi_amini committed changes. Now the patch should be good to go and code builds without warnings.

This revision was automatically updated to reflect the committed changes.