nrappkit/src/port/generic/include/sys/queue.h: '__offsetof' macro redefined on DragonFly

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jbeich, Assigned: jbeich)

Tracking

Trunk
mozilla32
x86_64
Other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

offsetof() macro in stddef.h is defined via __offsetof() from machine/stdint.h. sys/queue.h copy in nrappkit breaks this which makes the compiler treat offsetof() as implicitly declared function. But the build would break earlier due to invalid arguments to a function (vs. macro).

http://bxr.su/DragonFly/sys/cpu/x86_64/include/stdint.h
just use offsetof() like NetBSD and DragonFly
Attachment #805031 - Flags: review?(adam)
Posted patch use native <sys/queue.h> (obsolete) — Splinter Review
On FreeBSD we get:
- STAILQ_LAST() implemented via __member2struct()[1]
- a bug in _REMOVE fixed[2] and later merged to DragonFly

[1] http://svnweb.freebsd.org/changeset/base/240422
[2] http://svnweb.freebsd.org/changeset/base/204106

(In reply to Jan Beich from comment #1)
> just use offsetof() like NetBSD and DragonFly

oops, only NetBSD
Attachment #805032 - Flags: review?(adam)
Attachment #805031 - Flags: review?(adam) → review?(ekr)
Attachment #805032 - Flags: review?(adam) → review?(ekr)
Can you recommend someone else for review then? We have the patch in the main ports tree for some time now.

http://svnweb.freebsd.org/ports/head/www/firefox/files/patch-bug916216
https://github.com/DragonFlyBSD/DPorts/commits/master/www/firefox/files/patch-bug916589
Comment on attachment 805031 [details] [diff] [review]
avoid conflict

Review of attachment 805031 [details] [diff] [review]:
-----------------------------------------------------------------

re-assign to bwc
Attachment #805031 - Flags: review?(ekr) → review?(docfaraday)
Comment on attachment 805032 [details] [diff] [review]
use native <sys/queue.h>

Review of attachment 805032 [details] [diff] [review]:
-----------------------------------------------------------------

re-assign to bwc
Attachment #805032 - Flags: review?(ekr) → review?(docfaraday)
Comment on attachment 805032 [details] [diff] [review]
use native <sys/queue.h>

Review of attachment 805032 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/mtransport/third_party/nrappkit/src/port/generic/include/sys/queue.h
@@ +36,1 @@
>  #define	_SYS_QUEUE_H_

I'm left wondering if there is some problem with the implementation of sys/queue.h we have? We seem to be missing some amount of functionality, so maybe you're running into problems where you cannot use the full set of macros in your code? If so, as long as this passes try I guess I'd be ok with it (include_next seems to be supported on the toolchains we care about for BSD). Let me do a try push and see what happens.
Comment on attachment 805031 [details] [diff] [review]
avoid conflict

Review of attachment 805031 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/mtransport/third_party/nrappkit/src/port/generic/include/sys/queue.h
@@ +36,4 @@
>  #include <stddef.h>
> +
> +#ifndef offsetof
> +#define offsetof(type, field) ((size_t)(&((type *)0)->field))

On what platforms is this not present in stddef.h? I've tried looking around for anyone complaining about it being missing, and haven't found anything. Let me attempt a try run without this check at all.
Needinfo self to check back on the B2G KK build.
Flags: needinfo?(docfaraday)
Comment on attachment 805031 [details] [diff] [review]
avoid conflict

Review of attachment 805031 [details] [diff] [review]:
-----------------------------------------------------------------

Seems ok to me.
Attachment #805031 - Flags: review?(docfaraday) → review+
So, I just need a thing or two from you:

1) I just need to know what difficulty you ran into with sys/queue.h, since I don't want to rock the boat on this unless there's a specific reason.

2) I'll need you to update the commit messages on your patches so that they stand on their own somewhat ("avoid conflict" is too vague). The typical format the sheriffs like is "Bug <bugnum> - Part <N>: <description>". Additionally, if a patch has been r+, append a " r=<reviewer>" to the description (eg; r=bwc).
Flags: needinfo?(jbeich)
Oops, DragonFly uses _MUTABLE suffix for what's _SAFE on FreeBSD. Noticed after looking at DPorts. netwerk/sctp/ part is fixed by bug 916427.

https://github.com/DragonFlyBSD/DeltaPorts/blob/master/ports/www/firefox/Makefile.DragonFly

(In reply to Byron Campen [:bwc] from comment #7)
> :::
> media/mtransport/third_party/nrappkit/src/port/generic/include/sys/queue.h
> @@ +36,4 @@
> >  #include <stddef.h>
> > +
> > +#ifndef offsetof
> > +#define offsetof(type, field) ((size_t)(&((type *)0)->field))
> 
> On what platforms is this not present in stddef.h? I've tried looking around
> for anyone complaining about it being missing, and haven't found anything.
> Let me attempt a try run without this check at all.

Maybe Solaris (another Tier3), its source has a lot of inplace |#define offsetof|.

(In reply to Byron Campen [:bwc] from comment #11)
> So, I just need a thing or two from you:
> 
> 1) I just need to know what difficulty you ran into with sys/queue.h, since
> I don't want to rock the boat on this unless there's a specific reason.

|avoid conflict| patch is alternative fix to the kludge currently present in queue.h. I don't want to list *every* platform where __offsetof cannot be redefined.

In file included from media/mtransport/third_party/nrappkit/src/share/nr_common.h:66:0,
                 from media/mtransport/third_party/nrappkit/src/util/p_buf.c:45:
media/mtransport/third_party/nrappkit/src/port/generic/include/sys/queue.h:38:0: warning: "__offsetof" redefined [enabled by default]
In file included from /usr/include/sys/stdint.h:11:0,
                 from /usr/include/stdint.h:32,
                 from /usr/include/sys/types.h:49,
                 from /usr/include/string.h:38,
                 from media/mtransport/third_party/nrappkit/src/util/p_buf.c:43:
/usr/include/machine/stdint.h:136:0: note: this is the location of the previous definition
media/mtransport/third_party/nrappkit/src/util/p_buf.c: In function 'nr_p_buf_write_to_chain':
media/mtransport/third_party/nrappkit/src/util/p_buf.c:162:9: warning: implicit declaration of function 'offsetof' [-Wimplicit-function-declaration]
media/mtransport/third_party/nrappkit/src/util/p_buf.c:162:9: error: expected expression before 'struct'

BSDs tend to have newer version of queue(3) so using system implementation there makes debugging it or nrappkit/nICEr easier. My argument is probably weak here and the patch can be dropped.

> 
> 2) I'll need you to update the commit messages on your patches so that they
> stand on their own somewhat ("avoid conflict" is too vague). The typical
> format the sheriffs like is "Bug <bugnum> - Part <N>: <description>".
> Additionally, if a patch has been r+, append a " r=<reviewer>" to the
> description (eg; r=bwc).

attachment 805031 [details] [diff] [review] already contains a commit message with |Bug <bugnum>| where Mercurial header is generated by |git bz attach|. The short attachment description is supposed to give a hint to the reviewer or to more easily refer to the patch.

No |Part <N>| because the patches are somewhat independent. Either one can fix build on DragonFly and it doesn't hurt to apply both as well.

Just adding |r=<reviewer>| (which may change) and loosing r+ is kind of pointless. Ditto for |Bug <bugnum>| if you have a patch when opening a bug (unless bugzilla supports locking <bugnum> without wasting comment 0).
Attachment #805032 - Attachment is obsolete: true
Attachment #805032 - Flags: review?(docfaraday)
Attachment #8425839 - Flags: review?(docfaraday)
(In reply to Jan Beich from comment #12)
> Created attachment 8425839 [details] [diff] [review]
> use native <sys/queue.h>, v1.1
> 
> Oops, DragonFly uses _MUTABLE suffix for what's _SAFE on FreeBSD. Noticed
> after looking at DPorts. netwerk/sctp/ part is fixed by bug 916427.
> 
> https://github.com/DragonFlyBSD/DeltaPorts/blob/master/ports/www/firefox/
> Makefile.DragonFly

> |avoid conflict| patch is alternative fix to the kludge currently present in
> queue.h. I don't want to list *every* platform where __offsetof cannot be
> redefined.
> 

   Ok, I think I like the |avoid conflict| patch better, if it works for you.

> In file included from
> media/mtransport/third_party/nrappkit/src/share/nr_common.h:66:0,
>                  from
> media/mtransport/third_party/nrappkit/src/util/p_buf.c:45:
> media/mtransport/third_party/nrappkit/src/port/generic/include/sys/queue.h:
> 38:0: warning: "__offsetof" redefined [enabled by default]
> In file included from /usr/include/sys/stdint.h:11:0,
>                  from /usr/include/stdint.h:32,
>                  from /usr/include/sys/types.h:49,
>                  from /usr/include/string.h:38,
>                  from
> media/mtransport/third_party/nrappkit/src/util/p_buf.c:43:
> /usr/include/machine/stdint.h:136:0: note: this is the location of the
> previous definition
> media/mtransport/third_party/nrappkit/src/util/p_buf.c: In function
> 'nr_p_buf_write_to_chain':
> media/mtransport/third_party/nrappkit/src/util/p_buf.c:162:9: warning:
> implicit declaration of function 'offsetof' [-Wimplicit-function-declaration]
> media/mtransport/third_party/nrappkit/src/util/p_buf.c:162:9: error:
> expected expression before 'struct'
> 
> BSDs tend to have newer version of queue(3) so using system implementation
> there makes debugging it or nrappkit/nICEr easier. My argument is probably
> weak here and the patch can be dropped.
> 
> > 
> > 2) I'll need you to update the commit messages on your patches so that they
> > stand on their own somewhat ("avoid conflict" is too vague). The typical
> > format the sheriffs like is "Bug <bugnum> - Part <N>: <description>".
> > Additionally, if a patch has been r+, append a " r=<reviewer>" to the
> > description (eg; r=bwc).
> 
> attachment 805031 [details] [diff] [review] already contains a commit
> message with |Bug <bugnum>| where Mercurial header is generated by |git bz
> attach|. The short attachment description is supposed to give a hint to the
> reviewer or to more easily refer to the patch.
> 

   Ah, you're right, the patch description is not the same as the commit message in this case. Not used to that, no problem then.

> Just adding |r=<reviewer>| (which may change) and loosing r+ is kind of
> pointless. Ditto for |Bug <bugnum>| if you have a patch when opening a bug
> (unless bugzilla supports locking <bugnum> without wasting comment 0).

   Apologies, I was ambiguous; this is for when you update an already r+ed patch (for fixing nits like updating commit message, which is not necessary in this case as you've pointed out).
Flags: needinfo?(docfaraday)
Just to make sure we're on the same page; the |avoid conflict| patch looks ok to me, and I prefer it if it fixes the problem you're seeing. Are you ok with checking just that one in?
Comment on attachment 8425839 [details] [diff] [review]
use native <sys/queue.h>, v1.1

OK. nICEr/src/stun/addrs.c includes sys/sysctl.h which implicitly pulls sys/queue.h but that may only cause error if the file is built in unified mode with another file using _SAFE suffix.
Attachment #8425839 - Attachment is obsolete: true
Attachment #8425839 - Flags: review?(docfaraday)
Flags: needinfo?(jbeich)
Keywords: checkin-needed
Comment on attachment 805031 [details] [diff] [review]
avoid conflict

I want one less patch to carry around during esr31 lifetime.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 807492
User impact if declined: broken build on DragonFly (NPOTB platform) unless --disable-webrtc
Testing completed (on m-c, etc.): soon
Risk to taking this patch (and alternatives if risky): Low, only build errors.
String or IDL/UUID changes made by this patch: None
Attachment #805031 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b328f846f2ee
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #805031 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.