[erlang-patches] DTrace patch, review draft #3

[Björn-Egil Dahlberg]

On 2011-11-23 00:34, Scott Lystig Fritchie wrote:
> Henrik Nord<henrik@REDACTED>  wrote:
>
> hn>  There will be a R15A tag pushed to github later today. And I would
> hn>  like you to rebase your branch upon that, there is some minor merge
> hn>  conflicts.  Then I will refetch your branch tomorrow.
>
> I've rebased the "dtrace-review3" branch on the OTP_R15A tag as I found
> it today, commit e21ff9b0b69219ab3853be7e80813156113152b7.  Right now,
> the new head of the "dtrace-review3" branch is
> 4242b9c0eca65b767eabcc192d980fa3754d5b33.
>
> Many thanks for reviewing this work!
I am looking forward to dtrace in R15B-release.
Great work! =)

Review stuff:

I have only reviewed efile <- prim_file <- file (I had a major 
merge-confligt with the whole impl.-tree for opu =)

I do not like the file/efile dtrace implementation.
In prim_file:

read(FD, Size) ->                                                                                   

     read(FD, Size, get_dtrace_utag()).

get_dtrace_utag() which is a process dictionary get, not ideal for every 
read and write. Especially not for non-dtrace builds.

We have an idea for a solution. By introducing a special BIF that will 
return NIL for non-dtrace builds. This will be done by the loader so the 
compiler does not need to know anything (about that =).

Furthermore, the file driver should not receive extra data in non-dtrace 
builds (in this case it is only a byte though). This can be done by 
"#ifdef HAVE_DTRACE" on dt_utag = EV_CHAR_P(ev, p, q); etc. This is of 
course only viable with the loader solution above.

Compiler warnings:

Using gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1, i.e. no dtrace enabled
I think gcc's warning "unused-but-set-variable" is fairly new and a 
equipped with pain. I think the compiler normally optimizes this and 
removes it from the code. Still, it would be nice to remove the 
warnings. I get that some of them are on the TODO-list still, for 
instance erl_async len.
You can use ERTS_DECLARE_DUMMY(<Variable Declaration>); to suppress 
warnings, see erl_bif_binary.c for an example.


I get the following new warnings:

SLF common/erl_printf.c:179:5: warning: ISO C90 forbids mixed 
declarations and code [-Wdeclaration-after-statement]
SLF common/erl_printf.c:179:5: warning: ISO C90 forbids mixed 
declarations and code [-Wdeclaration-after-statement]

SLF beam/erl_message.c:339:42: warning: variable ‘tok_serial’ set but 
not used [-Wunused-but-set-variable]
SLF beam/erl_message.c:339:25: warning: variable ‘tok_lastcnt’ set but 
not used [-Wunused-but-set-variable]
SLF beam/erl_message.c:339:10: warning: variable ‘tok_label’ set but not 
used [-Wunused-but-set-variable]
SLF beam/erl_message.c:498:46: warning: variable ‘tok_serial’ set but 
not used [-Wunused-but-set-variable]
SLF beam/erl_message.c:498:29: warning: variable ‘tok_lastcnt’ set but 
not used [-Wunused-but-set-variable]
SLF beam/erl_message.c:498:14: warning: variable ‘tok_label’ set but not 
used [-Wunused-but-set-variable]
SLF beam/erl_message.c:825:42: warning: variable ‘tok_serial’ set but 
not used [-Wunused-but-set-variable]
SLF beam/erl_message.c:825:25: warning: variable ‘tok_lastcnt’ set but 
not used [-Wunused-but-set-variable]
SLF beam/erl_message.c:825:10: warning: variable ‘tok_label’ set but not 
used [-Wunused-but-set-variable]

SLF beam/dist.c:745:10: warning: variable ‘msize’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:744:42: warning: variable ‘tok_serial’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:744:25: warning: variable ‘tok_lastcnt’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:744:10: warning: variable ‘tok_label’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:792:12: warning: variable ‘msize’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:791:42: warning: variable ‘tok_serial’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:791:25: warning: variable ‘tok_lastcnt’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:791:10: warning: variable ‘tok_label’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:841:42: warning: variable ‘tok_serial’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:841:25: warning: variable ‘tok_lastcnt’ set but not used 
[-Wunused-but-set-variable]
SLF beam/dist.c:841:10: warning: variable ‘tok_label’ set but not used 
[-Wunused-but-set-variable]
SLF beam/erl_async.c:257:9: warning: unused variable ‘len’ 
[-Wunused-variable]
SLF beam/erl_async.c:292:9: warning: unused variable ‘len’ 
[-Wunused-variable]
SLF beam/erl_gc.c:2046:10: warning: unused variable ‘pidbuf’ 
[-Wunused-variable]
SLF beam/beam_emu.c:1966:47: warning: variable ‘tok_serial’ set but not 
used [-Wunused-but-set-variable]
SLF beam/beam_emu.c:1966:30: warning: variable ‘tok_lastcnt’ set but not 
used [-Wunused-but-set-variable]
SLF beam/beam_emu.c:1966:15: warning: variable ‘tok_label’ set but not 
used [-Wunused-but-set-variable]

SLF drivers/common/efile_drv.c:2320:45: warning: variable ‘dt_i4’ set 
but not used [-Wunused-but-set-variable]
SLF drivers/common/efile_drv.c:2320:34: warning: variable ‘dt_i3’ set 
but not used [-Wunused-but-set-variable]
SLF drivers/common/efile_drv.c:2320:23: warning: variable ‘dt_i2’ set 
but not used [-Wunused-but-set-variable]
SLF drivers/common/efile_drv.c:2320:12: warning: variable ‘dt_i1’ set 
but not used [-Wunused-but-set-variable]
SLF drivers/common/efile_drv.c:2319:11: warning: variable ‘dt_utag’ set 
but not used [-Wunused-but-set-variable]
SLF drivers/common/efile_drv.c:2843:28: warning: variable ‘dt_s1’ set 
but not used [-Wunused-but-set-variable]
SLF drivers/common/efile_drv.c:2842:45: warning: variable ‘dt_i4’ set 
but not used [-Wunused-but-set-variable]
SLF drivers/common/efile_drv.c:2842:45: warning: variable ‘dt_i4’ set 
but not used [-Wunused-but-set-variable]

Regards,
Björn-Egil