581478 - warning: "strdup" redefined

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Josh Matthews [:jdm]]

In file included from ../../../dist/include/nscore.h:51,
                 from /home/t_mattjo/src/firefox/mobilebase/widget/src/gtkxtbin/gtk2xtbin.h:48,
                 from /home/t_mattjo/src/firefox/mobilebase/widget/src/gtkxtbin/gtk2xtbin.c:46:
../../../dist/include/mozilla/mozalloc_macro_wrappers.h:60:1: warning: "strdup" redefined
In file included from /usr/include/string.h:635,
                 from ../../../dist/system_wrappers/string.h:3,
                 from /usr/include/X11/Intrinsic.h:64,
                 from ../../../dist/system_wrappers/X11/Intrinsic.h:3,
                 from /home/t_mattjo/src/firefox/mobilebase/widget/src/gtkxtbin/gtk2xtbin.h:44,
                 from /home/t_mattjo/src/firefox/mobilebase/widget/src/gtkxtbin/gtk2xtbin.c:46:
/usr/include/bits/string2.h:1316:1: warning: this is the location of the previous definition

Comment 1

[Karl Tomlinson (:karlt)]

Looks like mozalloc_macro_wrappers.h does nothing but insert another function call on the stack since bug 738176 landed and it didn't do much more before that.

The plan as of 2010 to make these use the infallible allocators hasn't been actioned, so it seems about time to remove this and mozalloc_undef_macro_wrappers.h.

Comment 2

[Daniel Holbert [:dholbert]]

I haven't hit this locally, but here's what I have in /usr/include/bits/string2.h at around line 1316, on my local machine:

> #  define __strdup(s) \
[...SNIP...]
> #  if defined __USE_SVID || defined __USE_BSD || defined __USE_XOPEN_EXTENDED
> #   define strdup(s) __strdup (s)
> #  endif
> # endif

so string2.h does indeed have a #define for strdup, and we're redefining it without ever #undef'ing it.
 
Disregarding comment 1 for the moment: a trivial band-aid for this (to allow bug 896292 to land) would be to just add an #ifdef / #undef before we (re)define strdup.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: band-aid

Per comment 2, this just #undef's strdup before we #define it, if it's already defined.

This should be functionally equivalent to what we're already doing, aside from the fact that it won't spam a warning.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Glandium had concerns over this approach on IRC last night, IIRC.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 781360 [details] [diff] [review]
band-aid

Tagging him for feedback, then... I'm not a macro wizard (my CS program shunned them as evil), but I don't immediately see why anything would be wrong with this.

Comment 6

[Mike Hommey [:glandium]]

My only concern is that it would only fix it in the cases where mozalloc_macro_wrappers.h is included after whatever includes /usr/include/bits/string2.h. But that's probably good enough.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 781360 [details] [diff] [review]
band-aid

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

::: memory/mozalloc/mozalloc_macro_wrappers.h
@@ +27,5 @@
> +/*
> + * Note: on some platforms, strdup may be a macro instead of a function.
> + * In that case, we have to #undef it to avoid warnings about redefining it.
> + */
> +#ifdef strdup

You don't strictly need the ifdef.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 781360 [details] [diff] [review]
band-aid

Let's make this a r+.

Comment 9

[Daniel Holbert [:dholbert]]

I removed the #ifdef, per glandium's suggestion, and pushed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/924c4d59db8b

karl, I don't know enough about mozmemory to respond intelligently to comment 1, but if there's anything we should do to address your points there, perhaps that can happen in a new bug?

Comment 10

[Daniel Holbert [:dholbert]]

[er, s/mozmemory/mozalloc/]

Comment 11

[Karl Tomlinson (:karlt)]

Comment 1 was mostly trying to say that we are wasting our time maintaining a header that does nothing beneficial, so the header should be removed.

The only problem with this bandaid, beyond comment 6, is that mozalloc_undef_macro_wrappers.h doesn't revert what mozalloc_macro_wrappers.h does, but mozalloc_undef_macro_wrappers.h isn't used anywhere critical anyway.

I don't have any objection to landing the bandaid.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/924c4d59db8b