Bug 32098

Summary: constness of argument names in XInternAtoms() is incorrect.
Product: xorg Reporter: Robert White <rwhite>
Component: Lib/XlibAssignee: Alan Coopersmith <alan.coopersmith>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium CC: remi
Version: git   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Robert White 2010-12-04 04:29:58 UTC
The type of the "names" argument to XInternAtoms has been changed from "char **" to "const char **". GCC chokes on this when passed "char **" values.

changing the signature to "char const * const *" or even "char const * const * const" would make "the correct thing" happen in the case where non-const values are being promoted to contness in the call to this function.

For Justification See:
http://www.parashift.com/c++-faq-lite/const-correctness.html#faq-18.17
Comment 1 Alan Coopersmith 2010-12-04 11:09:21 UTC
Except that Xlib is C, not C++, and C doesn't do that:
http://stackoverflow.com/questions/78125/why-cant-i-convert-char-to-a-const-char-const-in-c

If you have a working patch that fixes warnings with gcc, not g++,
I'd love to see it.
Comment 2 Robert White 2010-12-05 08:26:33 UTC
(In reply to comment #1)
> Except that Xlib is C, not C++, and C doesn't do that:
> http://stackoverflow.com/questions/78125/why-cant-i-convert-char-to-a-const-char-const-in-c
> 
> If you have a working patch that fixes warnings with gcc, not g++,
> I'd love to see it.

Good point...

Using: Func(const char *names[]); instead of Func(const char **names); does everything needed for both languages and silences the warnings and errors in each.  Least to my testing. It should be a drop-in semantic replacement in both cases. The use of an array instead of a pointer prevents modification of the resulting list-o-pointers and so prevents the possible error. This, in turn, propigates the immutable nature of the list safely and so is the same as the second const in char const * const *.
Comment 3 Robert White 2010-12-05 08:36:56 UTC
(In reply to comment #2)
> The use of an array instead of a pointer prevents modification of the
> resulting list-o-pointers and so prevents the possible error.

I was wrong about the why. using the array style doesn't prevent the modification of the pointers. It unambiguously propagates the "point to const char" type through the function call. This then prevents the accidental misuse of the constant pointer as a non-constant pointer.

Either way, the use of "const type **var" can almost always be more clearly expressed as "const type *var[]" as long as the called function doesn't get all upset about needing a local (or subscripting) to iterate over var.
Comment 4 Robert White 2010-12-05 09:50:38 UTC
Actually, you may even want void Func(char const * const names[]); I _think_ that will constantize both the character strings and the vector of pointers.
Comment 5 Alan Coopersmith 2010-12-05 10:19:20 UTC
Using the test case of the libXt/src/Selection.c code currently throwing
warnings over:

    static char* names[] = { ... };
    XInternAtoms(dpy, names, 4, FALSE, atoms);


This declaration in Xlib.h:

extern Status XInternAtoms(
    Display*		/* dpy */,
    _Xconst char * names[],
    int			/* count */,
    Bool		/* onlyIfExists */,
    Atom*		/* atoms_return */
);

Still results in gcc 3.4.3 reporting:
Selection.c: In function `GetPropList':
Selection.c:191: warning: passing arg 2 of `XInternAtoms' from incompatible pointer type

Sun Studio 12.1 reports:
"Selection.c", line 191: warning: argument #2 is incompatible with prototype:
        prototype: pointer to pointer to const char : "/net/also.us.oracle.com/export/alanc/X.Org/sx86/install/usr/X11R7/include/X11/Xlib.h", line 1549
        argument : pointer to pointer to char

With the Xlib.h declaration changed to:
    char const * const names[],

gcc 3.4.3 & 4.3.2 report:
Selection.c: In function `GetPropList':
Selection.c:191: warning: passing arg 2 of `XInternAtoms' from incompatible pointer type

Sun Studio 12.1 reports:
"Selection.c", line 191: warning: argument #2 is incompatible with prototype:
        prototype: pointer to const pointer to const char : "/net/also.us.oracle.com/export/alanc/X.Org/sx86/install/usr/X11R7/include/X11/Xlib.h", line 1549
        argument : pointer to pointer to char

Do you have an example of either of these declarations fixing warnings from
code that is currently issuing them?   (Xt may not be the best test case,
but its one we have)
Comment 6 Robert White 2010-12-05 13:54:17 UTC
Yep. Turns out I am an idiot, or at least I was last night. The code I was using to double-check my assertions contained one of the stupidest programming mistakes I've made in years.

Now I'm having flashbacks to an embedded platform compiler where I ran into this some years back and I ended up just dropping "const" from most virtually all of my vector of character handling.

I came to the conclusion back then that somewhere back in prehistory, when someone decided to make 'char * boo = "ya";' [e.g. silently permit/promote assigning a constant string to a non-constant character pointer] a legal construct, they did so because they realized that the "constness of pointer-to-pointer" problem was largely unsolvable in C.

What is odd is that "const char **name;" does have the "intermediate pointer movement problem" but "char const * const name[];" shouldn't as you cannot move name and you should not be able to alter the elements of name to redirect them.

I'll bet that this is why parameter "names" wasn't constant in XInternAtoms() to begin with... 

So like never-mind on my corrections.

But the addition of the new const is killing (build-breaking) a lot of applications out here in the real world (kdelibs, xulrunner, etc) so maybe it ought not be checked in just now...?
Comment 7 RĂ©mi Cardona 2010-12-13 13:30:35 UTC
Just piping in to say we've been bit by this as well.

https://bugs.gentoo.org/347603

@Alan, I'm not sure if this situation is solvable at all, C++ being much stricter. Wouldn't it be better to revert back to the old un-const signatures, at least until someone figures it out?

Cheers
Comment 9 Alan Coopersmith 2010-12-21 18:56:51 UTC
As Daniel pointed out on xorg-devel, the proposed solution requires
all libraries & the application being compiled to agree on the constness.
For lack of any good ideas at resolving, I've accepted defeat and reverted,
and we've all learned a lesson about constness in C being harder than it
appears:

http://cgit.freedesktop.org/xorg/lib/libX11/commit/?id=d2714d65e85b44abedf5f82e1a31506dba397ef2

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.