[DTrace-devel] [PATCH v2] Add support for inet_ntoa6() subroutine

[Eugene Loh]

I see this v2 patch in the mail archives, even though it never showed up in my inbox.  Ah well.

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
though there is some strsize weird thing noted below.

==== bpf/inet_ntoa6.S

Replace every instance of 0x3a with ':'.

Why the simple copyright banner?  No reference to Oracle Linux DTrace or UPL?

Thanks for the comment
   * We sadly cannot include struct declarations in assembler input files, so we
   * cannot use offsetof() to programmatically determine the offset of the rodata
   * member in the DTrace context (dt_ctx_t).  If that structure changes, the
   * following define must be updated as well.
but is there somewhere in dt_dctx.h where we can reference DCTX_RODATA to make this potential problem easier to find?  E.g., where "typedef struct dt_dctx {...} dt_dctx_t;" is defined, insert a comment that if the offset to rodata is changed, then the hard-coded value of DCTX_RODATA in bpf/inet_ntoa6.S needs to be updated.

There is the write_hex16 block comment:
       * Let s (%r0) be the number of digits output thus far.  It should be
       * incremented if it is non-zero or if the current digit is non-zero,
       * which can be expressed as (s + c) > 0.  We only advance the output
       * pointer if (s + c) > 0.
       *
       * To avoid branches, we calculate ((-(c + s)) >> 63) as the value to
       * add to the output pointer (and to s), becuse its value will be 0 iff
       * c and s are both 0, and 1 otherwise.
At least, s/becuse/because/.
But how about actually reducing the whole thing to:
       * Let s (%r0) be the number of digits output thus far.  It should be
       * incremented if it is non-zero or if the current digit c is non-zero.
       * This is achieved without branches by adding ((-(c + s)) >> 63) to
       * the output pointer (and to s).

Also in write_hex16, I see:
      add   %r3, %r0          /* %r3 = ((-(c + s)) >> 63) */
      neg   %r3
      rsh   %r3, 63                 /* 0 if '0', 1 otherwise */
What is '0'?  An ASCII char?  How about just skipping that comment, since this distinctive construct was just explained.

Thanks for documenting
   * %r9          dst
   * %r8          src
   * %r7          bitmap of non-zero words
   * %r6          dctx
   * [%fp-8]      tbloff
but how about
   * %r9       output pointer
   * %r8       input pointer
   * %r7       bitmap of non-zero words
   * %r6       dctx (ipv4 code path), temp reg (ipv6 code path)
   * [%fp-4]   tbloff
Note the s/8/4/ for tbloff.

I see:
       * We make use of the fact that dst is a tstring which is known to be
       * large enough to hold the longest output (OUTPUT_LEN = 40 bytes), and
       * two copies of the input data (2 * INPUT_LEN = 32 bytes).
How do we know a tstring is large enough?  In fact, I see
    $ dtrace -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l
    8
    $ dtrace -xstrsize=512 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l
    8
    $ dtrace -xstrsize=288 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l
    8
    $ dtrace -xstrsize=256 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l
    8
    $ dtrace -xstrsize=240 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l
    67117
    $ dtrace -xstrsize=224 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l
    67117
    $ dtrace -xstrsize=192 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l
    67117
    $ dtrace -xstrsize=128 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l
    67117
though presumably something else is going on other than just 40+2*16>strsize.

The rationale
   * The advantage of keeping the original copy
   * around is that we can use the unconverted words as input to
   * inet_ntoa() for mapped or embedded IPv4 addresses.
is a little funny since the original src could serve just as well.  I guess keeping src would consume another register, though.  Anyhow, how about just
   * The unconverted copy is retained to pass to dt_inet_ntoa.

In the comments, I'd avoid the labels "copy1" and "copy2".  They don't otherwise appear.  Plus, "copy1" is the second copy, while "copy2" is the first copy, which is kind of confusing.  How about "converted copy" and "unconverted copy," respectively.

In
     * The exact semantics are a bit fuzzy in that neither RFC 4291 nor
       * RFC 5952 address the case where a 6 zero-words are followed by 2
       * words that do not form a valid IPv4 address.  Legacy DTrace takes
       * the interpretation that a 6 zero-word prefix indicates an IPv4
       * IPv4-compatible IPv6 address, whereas e.g. glibc requires that the
       * last 2 words form a valid IPv4 address (i.e. first octet cannot be
       * zero).
the "IPv4" at the end of the fourth line looks redundant.

I see:
       * The implementation here adopts the legacy approach (in order):
The legacy implementation relies on __ipv6_addr_type(), which I *THINK* uses a slightly different order than what I see in bpf/inet_ntoa6.S, but I guess that's no big deal.

s/paarts/parts/

In
      /*
       * Determine the number of words to output at the start of the address.
       * It is found in the upper 4 bits in %r7 (result of the table lookup
       * above).  We place an upper bound to make the BPF verifier happy.
       */
      mov   %r6, %r7
      rsh   %r6, 4
      jgt   %r6, 7, .Ldone
how about replacing the branch with "and %r6, 7" instead.  One less branch.  After all, the point is to make the BPF verifier happy!

In
    .Ldone:
      /* Output the erminating NUL byte and return. */
that should be "terminating".

In
    .Lipv4:
      /* Output the last two words as an IPv2 address and return. */
      mov   %r1, %r6
      mov   %r2, %r8
      add   %r2, 16 + 12
      mov   %r3, %r9
      call  dt_inet_ntoa
how about
  s/IPv2/IPv4/
  s/16 + 12/INPUT_LEN + INPUT_LEN - 4/

==== libdtrace/dt_cg.c

I thought there were going to be some comments to explain the values in the table.

Actually, how about replacing those table values with code that generates the table?  That *might* be more compact, but in any case it gives a precise definition of the table contents, helping readers both to understand and to check the values.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.oracle.com/pipermail/dtrace-devel/attachments/20230805/5242ffe2/attachment-0001.html>