Page MenuHomeFreeBSD

libctf, dtrace: Plumb through support for CONSTVAL values
AcceptedPublic

Authored by cem on Feb 20 2019, 6:57 AM.

Details

Summary

Add CONSTVAL knowledge to userspace libctf (a different implementation from
ctfconvert/ctfmerge/ctfdump, apparently!) and libdtrace. Document in ctf.5.

Add 'constvalmask' on generic dtrace probe structure. When non-zero, it
specifies which cached args[] members are absent due to constant removal.
The other arguments must be shifted up and the real values inserted. This
is done in generic dtrace_probe(), using the provider-specified getargval()
POP.

The constvalmasks for functions in a module are populated during
fbt_provide_module(), the same routine the populates the fbt:MMM:: probes
for module MMM. (It is invoked at fbt load for existing linker_files, as
well as whenever a new .ko is loaded). MD fbt_provide_module_function()
routines call back into the MI routine fbt_update_constvalmask() to compute
and cache the constval bitmask for a given probe.

getargval() borrows a lot of CTF code from getargdesc() and there is
probably an opportunity to deduplicate that code somewhat. It determines if
the argument at a given index is a constant value type. If it is, it
returns that constant value. If it is not, it falls back to the ordinary
argument lookup logic (shifted by the appropriate number of constant value
arguments preceeding the given index).

TODO: tinderbox before commit, but the MI changes are fairly trivial and
copied directly from working x86.

Test Plan
static int my_x_y_z(char *buf, size_t bufsz, int a, b, c);
// bufsz always constant in callers

BEFORE

testvm# dtrace -vln 'fbt::my_x_y_z:entry'
...
        Argument Types
                args[0]: char *
                args[1]: int
                args[2]: int
                args[3]: int

testvm# dtrace -vn 'fbt::my_x_y_z:entry { print(args[0]); print(args[1]); print(args[2]); print(args[3]); print(args[4]); }' -c 'sysctl kern.testtest=1'
...
dtrace: pid 677 exited with status 1
CPU     ID                    FUNCTION:NAME
  3  17509                   my_x_y_z:entry char * 0xfffffe001579e660
int 0x2
int 0x3
int 0x4

AFTER

testvm# dtrace -vln 'fbt::my_x_y_z:entry'
...
        Argument Types
                args[0]: char *
                args[1]: size_t
                args[2]: int
                args[3]: int
                args[4]: int

testvm# dtrace -vn 'fbt::my_x_y_z:entry { print(args[0]); print(args[1]); print(args[2]); print(args[3]); print(args[4]); }' -c 'sysctl kern.testtest=1'
...
dtrace: pid 673 exited with status 1
CPU     ID                    FUNCTION:NAME
  2  17509                   my_x_y_z:entry char * 0xfffffe0016032660
size_t 0x100
int 0x2
int 0x3
int 0x4

(ISLN bugzilla ref: 145163)

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 22608
Build 21724: arc lint + arc unit

Event Timeline

cem created this revision.Feb 20 2019, 6:57 AM
cem edited the test plan for this revision. (Show Details)Feb 20 2019, 7:00 AM
markj added a comment.Feb 22 2019, 9:02 PM

I think the dtrace changes are somewhat hackish. Did you consider the approach of modifying the D compiler to shift argument indices accordingly when one of a probes arguments is a CONSTVAL?

cem added a comment.Feb 23 2019, 3:52 AM

Thanks for taking a look!

I think the dtrace changes are somewhat hackish.

Yes, that's a fair characterization. (I think it kind of fits in with the quality and design of the rest of fbt, but...)

Did you consider the approach of modifying the D compiler to shift argument indices accordingly when one of a probes arguments is a CONSTVAL?

No, I didn't consider it, aside from superficially. I was/am daunted by the task of understanding the D compiler well enough to make such a change to it without introducing a regression instead. The scope there seems larger. Maybe it isn't.

This approach was sort of a minimum viable PoC to test the soundness of the idea and try to get some feedback. I suspect the performance is adequate (we add a 99.99% correctly predicted branch in the hot path, some trivial one-time data structure analysis at module load, a few bytes in some runtime structures) and it's more or less functionally correct, if the ugliness of the design isn't a showstopper.

If the feedback is that the compiler would be a more suitable place for something like this, that's fine! After all, another somewhat common compiler optimization is reordering parameters, and this revision makes no attempt to deal with that, or even lay groundwork for solving both problems. However, I'm not sure when I would be able to take on the larger effort.

As I've said before, there's no real rush here. GENERIC has 2040 function types (across 32k functions and 79k parameters); 30,000 total types; and only 23 of these unique (distinct value & type) constval parameters. Throw a dart and you'd be unlikely to hit one of the routines broken. I'm happy to wait to do it right, once we figure out what that is.

In D19265#413165, @cem wrote:

Thanks for taking a look!

I think the dtrace changes are somewhat hackish.

Yes, that's a fair characterization. (I think it kind of fits in with the quality and design of the rest of fbt, but...)

Sorry, I was in a rush when I wrote that and should have said more. To me, this fix would ideally not touch the kernel components at all: the kernel's notion of function arguments should be limited to the ABI. Consider that the pid provider (fbt for userspace) has exactly this same problem, and this fix does not address that. fbt uses CTF solely for defining argument types when it registers probes, and even that's kind of a hack.

Did you consider the approach of modifying the D compiler to shift argument indices accordingly when one of a probes arguments is a CONSTVAL?

No, I didn't consider it, aside from superficially. I was/am daunted by the task of understanding the D compiler well enough to make such a change to it without introducing a regression instead. The scope there seems larger. Maybe it isn't.

Ok. I do think that that's probably harder to implement. I have not yet looked at how hard it would be to change the compiler, but I'll try to get a sense of it soon.

This approach was sort of a minimum viable PoC to test the soundness of the idea and try to get some feedback. I suspect the performance is adequate (we add a 99.99% correctly predicted branch in the hot path, some trivial one-time data structure analysis at module load, a few bytes in some runtime structures) and it's more or less functionally correct, if the ugliness of the design isn't a showstopper.
If the feedback is that the compiler would be a more suitable place for something like this, that's fine! After all, another somewhat common compiler optimization is reordering parameters, and this revision makes no attempt to deal with that, or even lay groundwork for solving both problems. However, I'm not sure when I would be able to take on the larger effort.

Right, I was thinking about that problem here too. I don't really remember the details of that one, but I think it had to do with passing a compound structure by value so that it occupies multiple argument registers?

Anyway, I don't mean to object to this patch since it obviously fixes a real problem, but I think it will need to be revisited eventually. When you commit, I'd ask that you at least commit the kernel changes separately.

As I've said before, there's no real rush here. GENERIC has 2040 function types (across 32k functions and 79k parameters); 30,000 total types; and only 23 of these unique (distinct value & type) constval parameters. Throw a dart and you'd be unlikely to hit one of the routines broken. I'm happy to wait to do it right, once we figure out what that is.

I think the CTF changes are going in the right direction at least.

cem added a comment.Feb 27 2019, 3:16 AM
In D19265#413165, @cem wrote:

Sorry, I was in a rush when I wrote that and should have said more. To me, this fix would ideally not touch the kernel components at all: the kernel's notion of function arguments should be limited to the ABI.

Sure, that sounds nice to me too. I'm not sure how we get there, though.

Consider that the pid provider (fbt for userspace) has exactly this same problem, and this fix does not address that.

Right; that is a shortcoming of this patch. We could probably fix pid::: in a similar, straightforward (but ugly) fashion. Are there any other Providers that rely on CTF function types?

fbt uses CTF solely for defining argument types when it registers probes, and even that's kind of a hack.

Right. Part of the problem of just leaving fbt to the actual ABI arguments is that its probe descriptions (dtrace -vln fbt::foo:entry) don't match the ABI now that we actually represent CONSTVAL parameters in CTF (previously, they were just discarded during ctfconvert). Maybe it's possible to leave that alone and capture ABI arguments, and have userspace Dtrace munge the captured argument values. But I am confused how it would all fit together nicely. Dtrace is kind of a mess.

Ok. I do think that that's probably harder to implement. I have not yet looked at how hard it would be to change the compiler, but I'll try to get a sense of it soon.

Thanks.

After all, another somewhat common compiler optimization is reordering parameters, and this revision makes no attempt to deal with that, or even lay groundwork for solving both problems.

Right, I was thinking about that problem here too. I don't really remember the details of that one, but I think it had to do with passing a compound structure by value so that it occupies multiple argument registers?

I don't recall why it would happen either. My best guesses are:

  1. small-word (32bit) architectures passing split multiword (64-bit) arguments: it may be more optimal to use certain pairs of registers to hold a logical 64-bit value
  2. something like tail-call optimization: if a subset of A()'s arguments are passed unmodified into B(), it might make sense to reorder the ABI of A (assuming it's local to a single CU) to put B's arguments first.

But I'm just speculating.

Anyway, I don't mean to object to this patch since it obviously fixes a real problem, but I think it will need to be revisited eventually. When you commit, I'd ask that you at least commit the kernel changes separately.

Sure; can I put you down for Reviewed-by the userspace portions? I can also look at expanding pid::: along with the fbt patch (and reducing code duplication somewhat).

I think the CTF changes are going in the right direction at least.

Cool!

markj accepted this revision.Feb 28 2019, 8:46 PM
In D19265#414685, @cem wrote:

Consider that the pid provider (fbt for userspace) has exactly this same problem, and this fix does not address that.

Right; that is a shortcoming of this patch. We could probably fix pid::: in a similar, straightforward (but ugly) fashion. Are there any other Providers that rely on CTF function types?

I can't think of any. fbt and pid are the "dynamic" providers, pretty much all the others define probes statically and provide type names which get resolved to CTF types by libdtrace. Presumably such a provider would never need to define a probe with an argument of type constval.

fbt uses CTF solely for defining argument types when it registers probes, and even that's kind of a hack.

Right. Part of the problem of just leaving fbt to the actual ABI arguments is that its probe descriptions (dtrace -vln fbt::foo:entry) don't match the ABI now that we actually represent CONSTVAL parameters in CTF (previously, they were just discarded during ctfconvert). Maybe it's possible to leave that alone and capture ABI arguments, and have userspace Dtrace munge the captured argument values. But I am confused how it would all fit together nicely. Dtrace is kind of a mess.

Well, it's ok for FBT's argument list and the ABI arguments not to line up, since libdtrace has all the information needed to translate between the two. My notion is that any time a script refers to a constval args[n], libdtrace would emit the "load immediate" DIF instruction which loads the constant value into a register, and any time it refers to a non-constval args[n+1], libdtrace knows to emit DIF with the ABI argument index shifted to take into account the fact that args[n] isn't represented in the ABI.

Ok. I do think that that's probably harder to implement. I have not yet looked at how hard it would be to change the compiler, but I'll try to get a sense of it soon.

Thanks.

After all, another somewhat common compiler optimization is reordering parameters, and this revision makes no attempt to deal with that, or even lay groundwork for solving both problems.

Right, I was thinking about that problem here too. I don't really remember the details of that one, but I think it had to do with passing a compound structure by value so that it occupies multiple argument registers?

I don't recall why it would happen either. My best guesses are:

  1. small-word (32bit) architectures passing split multiword (64-bit) arguments: it may be more optimal to use certain pairs of registers to hold a logical 64-bit value
  2. something like tail-call optimization: if a subset of A()'s arguments are passed unmodified into B(), it might make sense to reorder the ABI of A (assuming it's local to a single CU) to put B's arguments first.

I'm pretty sure there are one or several OneFS bugs around this; @rang_acm.org may be able to turn them up. In FreeBSD it hasn't come up since we basically never pass structures by value.

But I'm just speculating.

Anyway, I don't mean to object to this patch since it obviously fixes a real problem, but I think it will need to be revisited eventually. When you commit, I'd ask that you at least commit the kernel changes separately.

Sure; can I put you down for Reviewed-by the userspace portions? I can also look at expanding pid::: along with the fbt patch (and reducing code duplication somewhat).

Sure, I think the changes look ok. Did you run them through the dtrace test suite on OneFS?

Regarding the pid bits, it's up to you. CTF and the pid provider have some other issues related to the fact that strip(1) is not CTF aware, so I don't think the pid provider changes should block this one.

This revision is now accepted and ready to land.Feb 28 2019, 8:46 PM
cem added a comment.Mar 1 2019, 1:30 AM
In D19265#414685, @cem wrote:

My notion is that any time a script refers to a constval args[n], libdtrace would emit the "load immediate" DIF instruction which loads the constant value into a register, and any time it refers to a non-constval args[n+1], libdtrace knows to emit DIF with the ABI argument index shifted to take into account the fact that args[n] isn't represented in the ABI.

That sounds pretty reasonable. Any idea how difficult it would be to implement?

I'm pretty sure there are one or several OneFS bugs around this; @rang_acm.org may be able to turn them up. In FreeBSD it hasn't come up since we basically never pass structures by value.

Yeah, I vaguely remember them existing. I did a quick search, and here's what I have:

Clang 3.3 would actually incorrectly reorder the DWARF formal_parameters to put structure parameters at the end. The actual calling convention and ABI matched the source code; the DWARF info was just incorrect. This appears to be fixed in Clang as early as 3.9 (at least in the case Anton identified). So I don't think reordering is actually a concern at this time.

Sure; can I put you down for Reviewed-by the userspace portions? I can also look at expanding pid::: along with the fbt patch (and reducing code duplication somewhat).

Sure, I think the changes look ok. Did you run them through the dtrace test suite on OneFS?

No, I haven't tried to apply this to the OneFS tree at all yet or run any dtrace test suite.

Regarding the pid bits, it's up to you. CTF and the pid provider have some other issues related to the fact that strip(1) is not CTF aware, so I don't think the pid provider changes should block this one.

Ok.

markj added a comment.Mar 1 2019, 4:08 PM
In D19265#415623, @cem wrote:
In D19265#414685, @cem wrote:

My notion is that any time a script refers to a constval args[n], libdtrace would emit the "load immediate" DIF instruction which loads the constant value into a register, and any time it refers to a non-constval args[n+1], libdtrace knows to emit DIF with the ABI argument index shifted to take into account the fact that args[n] isn't represented in the ABI.

That sounds pretty reasonable. Any idea how difficult it would be to implement?

I haven't yet tried to figure that out.

I'm pretty sure there are one or several OneFS bugs around this; @rang_acm.org may be able to turn them up. In FreeBSD it hasn't come up since we basically never pass structures by value.

Yeah, I vaguely remember them existing. I did a quick search, and here's what I have:
Clang 3.3 would actually incorrectly reorder the DWARF formal_parameters to put structure parameters at the end. The actual calling convention and ABI matched the source code; the DWARF info was just incorrect. This appears to be fixed in Clang as early as 3.9 (at least in the case Anton identified). So I don't think reordering is actually a concern at this time.

Ah, right. I think there's still an issue in that dtrace expects probe arguments to fit in an ABI register.

Sure, I think the changes look ok. Did you run them through the dtrace test suite on OneFS?

No, I haven't tried to apply this to the OneFS tree at all yet or run any dtrace test suite.

Ah, I assumed this was a port from OneFS. I'll try running it the suite this afternoon.

cem added a comment.Mar 1 2019, 6:41 PM

My notion is that any time a script refers to a constval args[n], libdtrace would emit the "load immediate" DIF instruction which loads the constant value into a register, and any time it refers to a non-constval args[n+1], libdtrace knows to emit DIF with the ABI argument index shifted to take into account the fact that args[n] isn't represented in the ABI.

...
I haven't yet tried to figure that out.

Oh, ok. I think that might be pretty reasonable and maybe not too difficult. I need a short break from Solaris code, though, before I would attempt to tackle it myself.

One thing that occurred to me with the proposed approach is that kernel fbt will still need a subset of this differential's change, just to grok constvals for getargdesc. getargdesc is ABI-unaware and must be able to at least parse CONSTVAL ctf kinds from function argument specifications. We could elide them, and let libdtrace reconstruct them, or just encode them like an ordinary CONST-qualified type argument like we do today. But that's a smaller change, limited to fbt (and maybe pid), and less of a gross hack of global dtrace.

Ah, right. I think there's still an issue in that dtrace expects probe arguments to fit in an ABI register.

Probably true, although the function where Anton observed reordering in OneFS's struct parameter was a 64-bit struct (with kind of odd but valid layout: 16 bit member, 8 bit, 8 bit, 32 bit).

I think 32-bit architectures must have the same issue with e.g. off_t or paddr_t (PAE), right?

A good issue to resolve somehow, and probably related ("generate smarter DIF"), but also probably orthogonal to addressing this particular bug.

Ah, I assumed this was a port from OneFS. I'll try running the suite this afternoon.

Thanks!

markj added a comment.Mar 1 2019, 11:15 PM
In D19265#415876, @cem wrote:

Ah, I assumed this was a port from OneFS. I'll try running the suite this afternoon.

Thanks!

With these diffs applied I get a lockup when running fbtprovider/tst.basic.d. To get the test suites run make -C cddl/usr.sbin/dtrace/tests all install; they get put in /usr/tests/cddl/usr.sbin/dtrace/common. (You need to set WITH_DTRACE_TESTS=YES to get them installed at the moment.)

cem added a comment.Mar 1 2019, 11:36 PM

https://github.com/oracle/dtrace-utils/blob/master/test/stress/fbtprovider/tst.basic.d

So probably we're mishandling a request for an argument value beyond the end of the valid parameters list.