[DTrace-devel] [PATCH 04/12] Fix the -xcpp option

[Kris Van Hees]

On Sun, Jul 17, 2022 at 05:43:41PM -0700, Eugene Loh wrote:
> On 7/16/22 21:39, Kris Van Hees wrote:
> 
> > On Fri, Jul 15, 2022 at 07:37:17PM -0700, Eugene Loh via DTrace-devel wrote:
> > > First of all, sorry for screwing up the Reply-to: stuff.  A number of posted
> > > patches never appeared in my inbox.  So I'm posting my reviews as fresh
> > > email threads in those cases.
> > > 
> > > For this one:
> > >      Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> > > since it's good enough as is.
> > > 
> > > Nonetheless, I wanted to run an alternative by you.  If I understand
> > > correctly, we want dtrace options -C and -xcpp to be exactly the same
> > > thing.  So why not make them the same thing "at inception" rather than
> > > carrying them around separately and then trying to make them equivalent at
> > > the point of usage?
> > > 
> > > That is, how about replacing this patch with a "one-line" fix?  In
> > > cmd/dtrace.c, in the -x processing, add the statement:
> > >          if (strcmp(optarg, "cpp") == 0)
> > >                  g_cflags |= DTRACE_C_CPP;
> > > I think that eliminates the need for the DTRACE_C_DLIB stuff.  I understand
> > > there is something a little weird about doing this in the -x processing, but
> > > the reality is that the weirdness is dtrace's way of handling these options
> > > -- the proposed addition is simply the most direct way of dealing with this.
> > The thing is that cmd/dtrace.c is the default front-end for libdtrace, and
> > options like -C and -S are cmd/dtrace options.  The -x<opt> options are
> > lindtrace options, i.e. options that can be set through the API that libdtrace
> > provides.  Therefore, processing them in cmd/dtrace.c would break that entire
> > design.
> I don't really follow this -- e.g., what "break that entire design" means.
> So just a few comments here.

My comment about breaking the design applied to the design of cmd/dtrace
options vs libdtrace options.  That is something we need to support.  And
since your suggestion breaks that, I pointed that out.

> One is that it does not make sense for the same code to be replicated in
> multiple places -- in this case, in both
>     dtrace_program_fcompile()
>     dtrace_program_strcompile()
> After all, they're both setting up for a call to dt_compile(), which has
> access to both cflags and dtp.  So, the work can be done there, as far as
> cpp is concerned.  Or in a different function altogether that can be called
> from the two functions.  In any case, we should not be maintaining the same
> code in two places.

But you overlook the fact that dt_compile() is also called from the
dtrace_type_strcompile() and dtrace_type_fcompile() functions, and in those
cases you do *not* want -xcpp to take effect.

> Now, one might argue that those two functions need cflags set up not only
> for cpp but also for verbose.  That's true, but the verbose problem is much
> wider than that.  E.g., with the verbose patch:
>     # dtrace            -S -n 'BEGIN { exit(0) }' |& grep Disassembly
>     Disassembly of clause :::BEGIN, <dt_clause_0>:
>     # dtrace -xdisasm=1 -S -n 'BEGIN { exit(0) }' |& grep Disassembly
>     Disassembly of clause :::BEGIN, <dt_clause_0>:
>     # dtrace -xdisasm=2 -S -n 'BEGIN { exit(0) }' |& grep Disassembly
>     Disassembly of program dtrace:::ERROR, <dt_error>:
>     Disassembly of program dtrace:::BEGIN:
>     Disassembly of program dtrace:::END:
>     # dtrace -xdisasm=4 -S -n 'BEGIN { exit(0) }' |& grep Disassembly
>     Disassembly of linked program dtrace:::ERROR, <dt_error>:
>     Disassembly of linked program dtrace:::BEGIN:
>     Disassembly of linked program dtrace:::END:
>     # dtrace -xdisasm=8 -S -n 'BEGIN { exit(0) }' |& grep Disassembly
>     Disassembly of final program dtrace:::BEGIN:
>     Disassembly of final program dtrace:::END:
> That's great. As advertised, -S works.  But then:
>     # dtrace            -xverbose -n 'BEGIN { exit(0) }' |& grep Disassembly
>     Disassembly of clause :::BEGIN, <dt_clause_0>:
>     # dtrace -xdisasm=1 -xverbose -n 'BEGIN { exit(0) }' |& grep Disassembly
>     Disassembly of clause :::BEGIN, <dt_clause_0>:
>     # dtrace -xdisasm=2 -xverbose -n 'BEGIN { exit(0) }' |& grep Disassembly
>     # dtrace -xdisasm=4 -xverbose -n 'BEGIN { exit(0) }' |& grep Disassembly
>     # dtrace -xdisasm=8 -xverbose -n 'BEGIN { exit(0) }' |& grep Disassembly
> Oops.  The problem is that the proposed solution operates during
> compilation, but most disassembly phases are later.  So a different solution
> is need for verbose/S.

The 'verbose' option is specifically described as: "DIF verbose mode, which
shows each compiled DIF object (DIFO)."  This differs from the description of
the -S option: "Show the D compiler intermediate code. The D compiler writes a
report of the intermediate code that was generated for each D program to
stderr."

So, -xverbose refers to DIF object, which is different from D programs.  Now,
that is a matter of interpretation of course.  But it seemed significant to me
that the original developers chose the name 'verbose' rather than e.g. 'disasm'
or something else that indicated that this is meant to provide disassembler
output.  I therefore chose the interpretation that -xverbose is equivalent with
-S, i.e. providing the default disassembly listing.  And that provides the
disassembly for each clause which is equivalent to providing the disassembly
for each DIF object.  Higher levels of disassembly that can be selected using
-xdisasm=N combined with -S operate on D programs, i.e. one or more clauses.

In all, -xverbose is a very oddly named option for what it enables.  Having the
-xdisasm=N option modify it (as it does for -S) makes that poorly named option
even worse in my opinion.  So, I would prefer to support the behaviour as
documented without extending it.

I can add a comment in the implementation plan to indicate that -xverbose is
not modified by -xdisasm=N.

> Anyhow, I guess my main comments are:
> 
> *)  It does not make sense to maintain the same code in multiple locations.

See my comment above on why this is the case.

> *)  The proposed solution does not work for verbose.

See my explanation above on why that is the case,

> *)  When tests are introduced for cpp, they should be in final form,
>     without having to be amended in a subsequent patch (blank comment
> lines).

That is a mistake - those changes were indeed meant to have been included in
this pathch.  I will ensure they are.

> *)  When the cpp fix is introduced, it should be in final form,
>     without having to be amended in a subsequent patch.

I deliberately implemented this patch series to show the development.  With
just -xcpp to consider, the more generic approach introduced in the -xverbose
patch was not necessary.  Only when -xverbose was implemented did it become
clear that the same approach could be used for both.  The patch series reflects
that.


> > I'd say that cmd/dtrace could probably be modified to actually use the
> > libdtrace API rather than passing in flags in an entirely different way, but
> > that seems to be something that has a larger scope and would best be done with
> > a separate patch series that reworks the cflags handling.  For now, the point
> > is to make the option work which is what this point does.