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

[Eugene Loh]

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.

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.

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.

Anyhow, I guess my main comments are:

*)  It does not make sense to maintain the same code in multiple locations.

*)  The proposed solution does not work for verbose.

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

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

> 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.