Date: Wed, 26 Feb 2014 09:52:51 -0800 From: Justin Hibbits <jhibbits@freebsd.org> To: Mark Johnston <markj@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers <src-committers@freebsd.org> Subject: Re: svn commit: r262466 - head/sys/cddl/dev/systrace Message-ID: <CAHSQbTBxHV=zrbYh62AS2q0HB0tkLgo_3AOR76BD5tgZBTdmTQ@mail.gmail.com> In-Reply-To: <20140226013550.GA16841@raichu> References: <201402250258.s1P2wCDd060659@svn.freebsd.org> <CAHSQbTCWRToOxwZOnCi-TzC_4vHUjppH5Kpdp238oL7HWNFc0A@mail.gmail.com> <20140226013550.GA16841@raichu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 25, 2014 at 5:35 PM, Mark Johnston <markj@freebsd.org> wrote:
> On Tue, Feb 25, 2014 at 03:17:56PM -0800, Justin Hibbits wrote:
>> I think this broke powerpc building. I see the following build failure:
>>
>> cc1: warnings being treated as errors
>> /home/chmeee/freebsd/head/sys/modules/dtrace/systrace/../../../cddl/dev/systrace/systrace.c:
>> In function 'systrace_probe':
>> /home/chmeee/freebsd/head/sys/modules/dtrace/systrace/../../../cddl/dev/systrace/systrace.c:218:
>> warning: function called through a non-compatible type
>> /home/chmeee/freebsd/head/sys/modules/dtrace/systrace/../../../cddl/dev/systrace/systrace.c:218:
>> note: if this code is reached, the program will abort
>>
>
> Hi Justin,
>
> Sorry about this. I've reverted the commit.
>
> I realize that the change introduced undefined behaviour, but a similar
> trick is used elsewhere in the DTrace code to pass extra arguments at a
> probe site. Calling dtrace_probe() through a function pointer (patch
> below) makes the warning go away, but I don't really understand why.
> clang doesn't emit warnings in either case.
>
> Thanks,
> -Mark
>
> diff --git a/sys/cddl/dev/systrace/systrace.c b/sys/cddl/dev/systrace/systrace.c
> index 83f0793..5f4b82f 100644
> --- a/sys/cddl/dev/systrace/systrace.c
> +++ b/sys/cddl/dev/systrace/systrace.c
> @@ -168,8 +168,8 @@ static dtrace_pops_t systrace_pops = {
> static struct cdev *systrace_cdev;
> static dtrace_provider_id_t systrace_id;
>
> -typedef void (*systrace_dtrace_probe)(dtrace_id_t, uintptr_t, uintptr_t,
> - uintptr_t, uintptr_t, uintptr_t, uintptr_t, uintptr_t, uintptr_t);
> +typedef void (*systrace_probe_t)(dtrace_id_t, uintptr_t, uintptr_t, uintptr_t,
> + uintptr_t, uintptr_t, uintptr_t, uintptr_t, uintptr_t);
>
> #if !defined(LINUX_SYSTRACE)
> /*
> @@ -214,8 +214,9 @@ systrace_probe(u_int32_t id, int sysnum, struct sysent *sysent, void *params,
> }
>
> /* Process the probe using the converted argments. */
> - ((systrace_dtrace_probe)(dtrace_probe))(id, uargs[0], uargs[1],
> - uargs[2], uargs[3], uargs[4], uargs[5], uargs[6], uargs[7]);
> + systrace_probe_t probe = (systrace_probe_t)dtrace_probe;
> + probe(id, uargs[0], uargs[1], uargs[2], uargs[3], uargs[4],
> + uargs[5], uargs[6], uargs[7]);
> }
>
> #endif
Hi Mark,
I think this patch works because it circumvents gcc's variable
tracking. With the first patch, gcc knew the function that was being
called, and knew it was undefined behavior. With the second patch, it
only knows at that time that you're calling through a function
pointer. It's completely forgotten that the function pointer is
pointing to that function. Just a guess. I'll give it a go and let
you know if it still complains.
Thanks!
- Justin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHSQbTBxHV=zrbYh62AS2q0HB0tkLgo_3AOR76BD5tgZBTdmTQ>