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
Paths
| Differential D23766
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:
Diff Detail Event Timelinevleschuk updated this object. vleschuk added a child revision: D23767: DebugInfo: use llvm::DINode::DIFlags type for debug info flags.Aug 22 2016, 6:47 AM Comment Actions 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
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. Comment ActionsChanged type name according to naming conventions. Comment Actions 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. Comment Actions Can somebody please look at it please? Are there any comments/objections to my recent replies? Comment Actions 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)? Comment Actions
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 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. Comment Actions
Comment Actions Ah, I guess having DIFlags inside DINode helps scope the enumerators inside it (DINode::FlagZero rather than having "FlagZero" pollute the llvm namespace).
Comment Actions
Correct. I also wanted to move DIFlags outside DINode, but than reconsidered it. Comment Actions Got rid of DIFlagsUnderlying typedef, adopted DebugInfoMetadata users and relevant tests to use DIFlags enum.
Comment Actions 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). This revision is now accepted and ready to land.Sep 1 2016, 2:35 PM Comment Actions 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 edited edge metadata. Comment ActionsRemoved few deprecated tests which don't make sense with new type for flags. Comment Actions The conversation doesn't seem to appear here. So I repost (with few more details):
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. Comment Actions Hello all, if there are no objects, could someone commit this for me please? I don't have commit rights yet. Thanks in advance. Closed by commit rL280686: DebugInfo: use strongly typed enum for debug info flags (authored by mehdi_amini). · Explain WhySep 5 2016, 8:22 PM 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 Comment Actions
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. Comment Actions
default: return ""; The test failed because this change removed "default" case from getFlagsString() function. Comment Actions Got rid of the following warning:
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. Closed by commit rL280700: DebugInfo: use strongly typed enum for debug info flags (authored by lkholodov). · Explain WhySep 6 2016, 3:55 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 69802 include/llvm/IR/DIBuilder.h
include/llvm/IR/DebugInfoFlags.def
include/llvm/IR/DebugInfoMetadata.h
lib/AsmParser/LLParser.cpp
lib/Bitcode/Reader/BitcodeReader.cpp
lib/IR/AsmWriter.cpp
lib/IR/DIBuilder.cpp
lib/IR/DebugInfoMetadata.cpp
unittests/IR/DebugInfoTest.cpp
unittests/IR/DebugTypeODRUniquingTest.cpp
unittests/IR/IRBuilderTest.cpp
unittests/IR/MetadataTest.cpp
unittests/Transforms/Utils/Cloning.cpp
|
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)