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

[Kris Van Hees]

On Mon, Jul 18, 2022 at 09:16:33AM -0700, Eugene Loh wrote:
> First (these comments apply more to the -xverbose patch, but we've been
> discussing these issues here):
> 
> 1)  It seems we should NOT say that -xverbose and -s are equivalent in light of
> their different behaviors and your explanations below how they are so
> different.

Well, they are not really that different but they do differ, so as I mention
below I will mention it in the implementation plan and commit message.

> 2)  Even if we want something to take place at multiple sites, we can maintain
> that "something" in one place and then call it from however many places we
> want.

Yes, but e.g. creating an intermediary function just to do that seems a bit of
overkill.  It's a single statement that appers in two functions.

> And...
> 
> On 7/17/22 21:19, Kris Van Hees wrote:
> 
>     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:
> 
>                 For this one:
>                      Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>                 since it's good enough as is.
> 
>         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.
> 
> 
> I'm puzzled by this objection.  Putting the check in dt_compile() is precisely
> what this patch does, isn't it?  Are you objecting to this patch?

I apologize - since you mentioned both cpp and verbose in your original email,
I combined them mentally in my replies.  For cpp, it was fine to have this in
dt_compile but when adding support for verbose, I needed to move it.

>     I can add a comment in the implementation plan to indicate that -xverbose is
>     not modified by -xdisasm=N.
> 
> I guess that's okay, but it would be better if explanations were part of more
> persistent artifacts -- at least commit messages and code but ideally user
> docs.  And, again, it sounds like we simply should not say that -S and
> -xverbose are equivalent.

Well, they are and they are not.  I guess we should call them related.
And by mentioning it in the implementation plan, we should use that (with
proper annotation) to drive doc changes when those are done.

I will explain a bit more in the commit message.

>         *)  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 guess I do not see the value of:
> 
> *)  Using the same solution for both problems.

Because it is using the same mechanism (cflags passed from cmd/dtrcae needs to
be adjusted based on dtp->dt_cflags which are set through the options API of
libdtrace).

> *)  Using a solution in one patch that is backed out in (basically) the next
> patch.  Progressive development is nice, but in this case it's "add code, now
> remove it."

These is not a tightly coupled patch series.  The -xcpp patch can stand on its
own, and I honestly do not think it necessarily would make "sense" to use the
combined implementation for it unless you already "know" that we are going to
be needing it for -xverbose.  I prefer to let it stand on its own merit, and
go through the adjustment later in the -xverbose patch to show that this new
fix requires more than what was provided for -xcpp.

> Anyhow, thanks for the discussion.

You're always welcome!  Discussion is good.