Page MenuHomePhabricator

[DebugInfo] Combine Trivial and NonTrivial flags
ClosedPublic

Authored by asmith on Mar 13 2019, 10:22 PM.

Details

Summary

These flags are used when emitting debug info and needed to initialize subprogram and member function attributes (function options) for Codeview. These function options are used to create an accurate compiler type for UDT symbols (class/struct/union) from PDBs.

The Trivial flag was introduced in https://reviews.llvm.org/D45122

It's been pointed out that Trivial and NonTrivial may imply each other and that seems to be the case in the current tests. This change combines them into a single flag -- NonTrivial -- and updates the corresponding unit tests. There is an additional change to llvm to update the flags.

Diff Detail

Repository
rL LLVM

Event Timeline

asmith created this revision.Mar 13 2019, 10:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2019, 10:22 PM

I'd preserve the trivial test cases and show that the NonTrivial flag is *not* present. I can suggest ways to achieve this if you like (note there is no CHECK-DAG-NOT combined directive).

test/CodeGenCXX/debug-info-composite-triviality.cpp
88 ↗(On Diff #190560)

This one used to be flagged nontrivial, and now it's not flagged?

Would it be simpler/better to revert all the FlagTrivial work? & use the FlagNonTrivial+composite type to imply trivial? (since FlagnonTrivial has been in-tree longer)

Either way. It was fewer changes to use FlagNonTrivial. Which flag do people want to keep?

As a rule I would prefer flags with positive names, as it's slightly easier to read !isTrivial than !isNonTrivial. And trivially shorter. :-)

@asmith: Where's the LLVM-side change/review that goes with this, btw?

As a rule I would prefer flags with positive names, as it's slightly easier to read !isTrivial than !isNonTrivial. And trivially shorter. :-)

Fair enough - I was mostly coming at this from the "the patch that was committed should be reverted" & then we could haggle over other things, but fair point.

Okay if we change the flag then I believe the tests under llvm/tests/lib/DebugInfo/CodeView must be updated. If we use NonTrivial all the existing tests work as expected.

LLVM change is here (you are all reviewers).
https://reviews.llvm.org/D59348

@asmith: Where's the LLVM-side change/review that goes with this, btw?

As a rule I would prefer flags with positive names, as it's slightly easier to read !isTrivial than !isNonTrivial. And trivially shorter. :-)

Fair enough - I was mostly coming at this from the "the patch that was committed should be reverted" & then we could haggle over other things, but fair point.

Hmm, one other thought: Technically "non trivial" is perhaps more accurate/less error prone. Only marking structures as "trivial" but other types without that marker makes it more subtle (since not all trivial types would be marked trivial - only those of a classification that means they /could/ be non-trivial). Whereas marking the non-trivial types is more broadly accurate.

@asmith - is this patch now essentially a revert of the trivial flag addition? Or are there any parts that were not reverted, if so, why not? (I would expect/imagine all the testing could be reverted too - since the NonTrivial flag was presumably already tested appropriately?)

@asmith: Where's the LLVM-side change/review that goes with this, btw?

As a rule I would prefer flags with positive names, as it's slightly easier to read !isTrivial than !isNonTrivial. And trivially shorter. :-)

Fair enough - I was mostly coming at this from the "the patch that was committed should be reverted" & then we could haggle over other things, but fair point.

Hmm, one other thought: Technically "non trivial" is perhaps more accurate/less error prone. Only marking structures as "trivial" but other types without that marker makes it more subtle (since not all trivial types would be marked trivial - only those of a classification that means they /could/ be non-trivial). Whereas marking the non-trivial types is more broadly accurate.

Well, that's a reasonable point in favor of keeping the NonTrivial flag.

asmith added a reviewer: Hui.Wed, Mar 27, 11:37 AM
Hui added inline comments.Wed, Apr 3, 2:19 PM
test/CodeGenCXX/debug-info-composite-triviality.cpp
88 ↗(On Diff #190560)

The base struct 'Trivial' was removed and this case became duplicated since it is now similar with 'struct NonTrivialD'. Could be very easy to add it back per request.

probinson added inline comments.Sun, Apr 7, 7:23 AM
test/CodeGenCXX/debug-info-composite-triviality.cpp
88 ↗(On Diff #190560)

I think it's worth having a multiple-inheritance case.

asmith updated this revision to Diff 194249.Mon, Apr 8, 11:43 PM
asmith marked 2 inline comments as done.Tue, Apr 9, 10:30 PM

This should be ready to commit

dblaikie accepted this revision.Wed, Apr 10, 2:31 PM

Approved pending the LLVM side changes/discussion.

This revision is now accepted and ready to land.Wed, Apr 10, 2:31 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 11, 1:23 PM