This is an archive of the discontinued LLVM Phabricator instance.

[llvm-debuginfo-analyzer] Add combined selection for logical elements.
Needs ReviewPublic

Authored by CarlosAlbertoEnciso on Apr 20 2023, 1:07 AM.

Details

Summary

Sometimes the output generated by any of the options:

--select=<pattern>
--select-elements=<condition[,condition,...]>
--select-lines=<kind[,kind,...]>
--select-scopes=<kind[,kind,...]>
--select-symbols=<kind[,kind,...]>
--select-types=<kind[,kind,...]>

is too general and includes information that is not required.

Add a combined selection feature, that includes the <pattern> AND <kind> as a criteria, to allow the selection of a specific <kind>
of logical element, line, scope, symbol or type that matches the given <pattern>.

--select=<pattern> --select-elements=<condition[,condition,...]>
--select=<pattern> --select-lines=<kind[,kind,...]>
--select=<pattern> --select-scopes=<kind[,kind,...]>
--select=<pattern> --select-symbols=<kind[,kind,...]>
--select=<pattern> --select-types=<kind[,kind,...]>

For the --report=view, include only elements that meet the criteria.

Diff Detail

Event Timeline

CarlosAlbertoEnciso requested review of this revision.Apr 20 2023, 1:07 AM
jmorse added a comment.May 5 2023, 3:53 AM

Sounds good, some questions / suggestions inline

llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst
467–469

Good place to forward-reference the filtering/combined section?

581

Could I suggest that this section is titled and described in terms of "filter" rather than "combined selection" -- I think the meaning of "filter" is clear to all readers, and it'll be easier + quicker to understand the command line options if written in those terms.

As far as I understand it, all of the options you're adding are to reduce the number of things printed by filtering for a particular condition, right?

632

What do you mean by mix here -- you can use multiple --select-something options, or you need to add pairs of --select and --select-something?

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
545–546

It's not completely clear to me what's going on here (docu-comments appreciated), in particular, shouldn't the getSelectCombinedPattern case be a stricter version of the other side of the branch? And if so, why is it different, can't it be implemented as

if (doesnt-match-filter)
    return;
the-rest-of-the-body();

?

llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst
467–469

Added a forward-reference to FILTER section.

581

Section changed to FILTER.

Your understanding is correct.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
545–546

'SelectCombinedPattern` is set when:

(--select-elements or --select-lines or --select-scopes or --select-symbols or --select-types) and (--select) are specified in the command line.

The matching conditions for both branches are different.
I can remove the else branch and add return on the if branch to simplify the code. Basically the combined takes priority over the normal selection.

llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst
632

Yes. You can use multiple --select-something options and multiple --select. They don't need to be paired.

@jmorse Thanks for your review. I have addressed the raised points.

jmorse added a comment.Jun 6 2023, 4:25 AM

I've got a reasonable amount of confusion here -- what's currently line 634 reads "for more specific combined criterias", and further up you agree that the point of the option is to "filter" and reduce the number of elements presented. However the explanatory text,

A logical element, line, scope, symbol or type will be selected if their name or line number matches any of the given <pattern>s **and** their <kind> or <condition> matches any of the given <kind>s or <condition>s.

actually broadens the number of elements presented: given that only one "--select" pattern needs to be matched, additional "--select" patterns will expand the number of elements presented, and it's the same for the "kind" selections. Am I misunderstanding this or are the two opposed; and if so, which is it supposed to be? I can see use-cases for both behaviours.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
512–520

This feels more like documentation for the option, rather than the following function, resolveGenericPatternMatch -- it's the purpose of the function that's unclear to me.