Page MenuHomeFreeBSD

ctf: Add support for typed constant values
AcceptedPublic

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

Details

Summary

Sometimes, as an optimization, compilers will lift out known-constant
function parameters from the actual calling convention. This is represented
by function argument debuginfo (DW_TAG_formal_parameter) having a
DW_AT_const_value attribute instead of a DW_AT_location.

In order for dtrace to understand such functions, we need to represent it
somehow in CTF. This is one scheme for doing so. An alternative might be
setting some sort of flag on the FUNCTION object (or adding a FUNCTION2
type) and adding the reordering and constant elimination logic there.

This commit does not add dtrace support for this new CONSTVAL type, just
ctfconvert, ctfmerge, and ctfdump.

Diff Detail

Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22814
Build 21903: arc lint + arc unit

Event Timeline

cem created this revision.Feb 20 2019, 6:50 AM
markj added a comment.Feb 20 2019, 4:08 PM

Can you provide an example DIE where this occurs? Which compiler are you using?

cem added a comment.Feb 20 2019, 4:49 PM

Can you provide an example DIE where this occurs? Which compiler are you using?

Sure. In AMD64 GENERIC w/ Clang 7.0.1, there are 23 of these in the linked kernel (maybe more in modules). Anton originally reported this against Clang 3.3, so it's not a new optimization. (ctfconvert + GCC6 has larger, orthogonal issues that I'd like to see addressed as well, but those are outside the scope of this change.)

One example is kern_chflagsat().

ctfdump (after this revision):

- Functions ------------------------------------------------------------------
...
[9621] FUNC (kern_chflagsat) returns: 3 args: (380, 3, 114, 4269, 329, 3)
...
- Types ----------------------------------------------------------------------
...
  <3> INTEGER int encoding=SIGNED offset=0 bits=32
...
  <15> INTEGER char encoding=SIGNED CHAR offset=0 bits=8
...
  [113] CONST (anon) refers to 15
  <114> POINTER (anon) refers to 113
...
  <329> TYPEDEF u_long refers to 17
...
  <379> STRUCT thread (1368 bytes)
        td_lock type=238 off=0
...
  <380> POINTER (anon) refers to 379
...
  <576> ENUM uio_seg
        UIO_USERSPACE = 0
        UIO_SYSSPACE = 1
        UIO_NOCOPY = 2
...
  [4269] CONSTVAL 0 refers to: 576

I.e., the 4th argument to kern_chflagsat is always the constant UIO_USERSPACE.

llvm-dwarfdump60 on vfs_syscalls.o:

0x0000d6f1:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000005140)
                DW_AT_high_pc   (0x0000000000005224)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_name      ("kern_chflagsat")
                DW_AT_decl_file ("/usr/home/conrad/src/freebsd/sys/kern/vfs_syscalls.c")
                DW_AT_decl_line (2653)
                DW_AT_prototyped        (0x01)
                DW_AT_type      (cu + 0x01b0 "int")

0x0000d710:     DW_TAG_formal_parameter
                  DW_AT_name    ("td")
                  DW_AT_type    (cu + 0x0de8 "thread*")
                  ...

0x0000d720:     DW_TAG_formal_parameter
                  DW_AT_name    ("fd")
                  DW_AT_type    (cu + 0x01b0 "int")
                  ...

0x0000d730:     DW_TAG_formal_parameter
                  DW_AT_name    ("path")
                  DW_AT_type    (cu + 0x00aa "const char*")
                  ...

0x0000d740:     DW_TAG_formal_parameter
                  DW_AT_const_value     (0)                         //  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
                  DW_AT_name    ("pathseg")
                  DW_AT_decl_file       ("/usr/home/conrad/src/freebsd/sys/kern/vfs_syscalls.c")
                  DW_AT_decl_line       (2654)
                  DW_AT_type    (cu + 0x3535 "uio_seg")

0x0000d74d:     DW_TAG_formal_parameter
                  DW_AT_name    ("flags")
                  DW_AT_type    (cu + 0x0098 "u_long")
                  ...

0x0000d75d:     DW_TAG_formal_parameter
                  DW_AT_name    ("atflag")
                  DW_AT_type    (cu + 0x01b0 "int")
                  ...

And from dtrace (child revision):

testvm# dtrace -vln fbt::kern_chflagsat:entry
   ID   PROVIDER            MODULE                          FUNCTION NAME
17563        fbt            kernel                    kern_chflagsat entry
...
        Argument Types
                args[0]: struct thread *
                args[1]: int
                args[2]: char *
                args[3]: enum uio_seg
                args[4]: u_long
                args[5]: int

(The const-ness of the uio_seg isn't exactly captured in -vl yet, but maybe it could be. I plumbed it through ctf_type_qlname in a couple places but it seems like dtrace has a couple copies of various code in the tree, which is unfortunate.)

cem added a comment.Feb 20 2019, 4:59 PM

Prior to this change, ctfdump shows [2] FUNC (kern_chflagsat) returns: 1 args: (1173, 1, 5, 142, 91, 1); ... <142> ENUM uio_seg (i.e., no idea it's been const-ed out of the ABI).

markj added a comment.Feb 22 2019, 8:55 PM

Overall this looks ok to me.

cddl/contrib/opensolaris/tools/ctf/cvt/dwarf.c
546

If you handle the !hasconst case first, you can de-indent the rest of the function.

cddl/contrib/opensolaris/tools/ctf/cvt/output.c
98

Use designated initializers here too?

cddl/contrib/opensolaris/tools/ctf/cvt/tdata.c
71

I don't really understand this. What's the name of a constval? Why would two constvals with the same name be equivalent?

479

Designated initializers?

cem added inline comments.Feb 23 2019, 3:34 AM
cddl/contrib/opensolaris/tools/ctf/cvt/dwarf.c
546

sure, will do

cddl/contrib/opensolaris/tools/ctf/cvt/output.c
98

Sure, I can do that

cddl/contrib/opensolaris/tools/ctf/cvt/tdata.c
71

It's possible this change is errneous.

In many ways constval is similar to other qualifying ctf types like restrict/const/volatile/pointer, so a lot of this change was looking for places where behavior is changed based on kind and adding constval if it looked needed. I don't recall how I decided to add it here.

You're right — the underlying type name doesn't indicate two constvals are equivalent, which is why we had to add tdesc_cvcmp().

479

will do

cem updated this revision to Diff 54305.Feb 24 2019, 6:30 PM
cem marked 3 inline comments as done.
  • Pull out !hasconst case and de-indent
  • Use designated initializers in a few more places
markj accepted this revision.Feb 28 2019, 8:31 PM
markj added inline comments.
cddl/contrib/opensolaris/tools/ctf/cvt/merge.c
578

More places to use a designated initializer.

cddl/contrib/opensolaris/tools/ctf/cvt/tdata.c
71

Ok, I understand it better now. This is just used for hashing; I think your change here is ok.

cddl/contrib/opensolaris/tools/ctf/dump/dump.c
637

"refers to:" sounds a bit odd to me. May "of type:"?

This revision is now accepted and ready to land.Feb 28 2019, 8:31 PM
cem added inline comments.Feb 28 2019, 11:17 PM
cddl/contrib/opensolaris/tools/ctf/cvt/merge.c
578

Sure, can do

cddl/contrib/opensolaris/tools/ctf/dump/dump.c
637

It's following the existing convention as far as printing the reference to the next type in the CTF typechain. ctt_name is always "(anon)" except for typedefs, AFAICT.

[76] FUNCTION (anon) returns: 14 args: (67, 14, 14)
[77] POINTER (anon) refers to 76
[78] CONST (anon) refers to 77

A CONSTVAL points to some arbitrary other type, which may referentially point to any number of other types. All of the types in the chain together describe the type of the value. So I'm not sure "CONSTVAL 7 of type: 23" makes too much more sense than "CONSTVAL 7 refers to 23", given the existing pattern.

markj added inline comments.Feb 28 2019, 11:24 PM
cddl/contrib/opensolaris/tools/ctf/dump/dump.c
637

Right, but POINTER, CONST, etc., are qualifiers which necessarily refer to another type. CONSTVALs are instances of a type.

I don't feel strongly about it, but I don't think there's a pattern to follow here.

cem added inline comments.Mar 1 2019, 12:46 AM
cddl/contrib/opensolaris/tools/ctf/dump/dump.c
637

That's reasonable. I'll spend a few minutes and see if I can come up with anything I like better; otherwise steal your suggestion directly.

cem updated this revision to Diff 54561.Mar 1 2019, 1:05 AM
cem marked 3 inline comments as done.

Use C99 initializers in a few more dispatch tables and clarify wording of ctfdump CONSTVAL type output

This revision now requires review to proceed.Mar 1 2019, 1:05 AM
markj accepted this revision.Mar 1 2019, 4:05 PM
markj added inline comments.
cddl/contrib/opensolaris/tools/ctf/dump/dump.c
637

Thanks.

This revision is now accepted and ready to land.Mar 1 2019, 4:05 PM