84551 - "Wrong" prototype possibly generated with "string arrays" in IDL

Status: NEW

(Core :: XPCOM, defect, P5)

Description

[Leif Hedstrom]

Hiya,

I had some compile time errors with an interface, declared in IDL as:

    void setAttributes(in unsigned long aCount,
                       [array, size_is(aCount)] in string aAttrs);


This generates this prototype in the |.h| file:

NS_IMETHODIMP nsLDAPURL::SetAttributes(PRUint32 aCount, const char **aAttrs)


Alright, so, in a function calling this method, I have a |char **| variable, in
my case called |desc->lud_attrs| (it's part of the LDAP C-SDK), and the call is

    rv = SetAttributes(count, desc->lud_attrs);


The compiler did not like this, and I had to enforce some casting on my
variable. I talked to SCC, and he indicates that this is not the right prototype
for my Setter function, I'm pasting our discussion from IRC below.

The |desc| is declared as |LDAPURLDesc *desc;| and LDAPURLDesc is declared in
the LDAP C-SDK headers like:

typedef struct ldap_url_desc {
    char                *lud_host;
    int                 lud_port;
    char                *lud_dn;
    char                **lud_attrs;
    int                 lud_scope;
    char                *lud_filter;
    unsigned long       lud_options;
#define LDAP_URL_OPT_SECURE     0x01
    char        *lud_string;    /* for internal use only */
} LDAPURLDesc;


Thanks,

-- Leif

------ From IRC disccusion with SCC -------------

<leif> Ok, in a module (nsLDAPURL.cpp / nsILDAPURL.idl), I have:
<leif> / void setAttributes (in unsigned long aCount,
<leif> /                     [array, size_is (aCount)] in string attrs); */
<leif> NS_IMETHODIMP nsLDAPURL::SetAttributes(PRUint32 count, const char
**aAttrs)
<leif> {
<leif>     PRUint32 index = 0;
<leif> This all works well, and it's scriptable etc.
<leif> Now, I need to call this function from within another function in the
same module. Like
<leif> rv = SetAttributes(count, desc->lud_attrs);
<leif> where desc->lud_attrs is a |char **| (not const)
<leif> the compiler doesn't like this... Adding a cast, like (const char **) in
front of |desc->lud_attrs| helps, but it's wrong, right?
<scc> hmmm
<scc> what compiler?
<leif> gcc
<scc> hmmm
<leif> It complains that there's no matching function, but points to my function
and says it's a "close match"
<scc> |const char**| means a pointer to a pointer to characters I won't change
<leif> right
<scc> that's how |SetAttributes| intends to use the array
<leif> which makes sense, in the Setter, the array ought to be read only (I copy
the strings)
<scc> the problem is, I think
<scc>  that the array is really mis-declared
<scc> it should be something like
<scc> |const char *const * aAttrs|
<scc> is |desc| |const| in this case?
<scc> if so, that might be why your compiler is complaining
<leif> hmmm, well, I just "copied and pasted" that prototype from the generated
.h file (generated by xpidl I assume)
<scc> because it thinks |SetAttributes| won't touch the characters themselves,
but might want to put a new pointer into the array
<leif> can I change that in the IDL file?
<scc> and you passed it an array that you had only |const| access to the
elements in
<scc> well, first, I would test this theory
<leif> alright, lemme try
<scc> modify the generated .h file
<leif> yes, that compiles
<leif> now doing |rv = SetAttributes(count, desc->lud_attrs);| works fine
<scc> ok, so this is really a bug in IDL
<leif> :(
<scc> the array declaration should have generated the thing I said
<scc> file a bug on jband, cc me
<leif> Ok. What do I do in the mean time?
<scc> paste this exchange into it
<scc> hmmm, ...thinking...
<scc> well, try this as a temporary measure
<scc> NS_CONST_CAST(const char**, desk->lud_attrs)
<scc> that's a slightly safer way of the cast you were already doing
<scc> if it works

Comment 1

[Scott Collins]

An `in' array should not only be an array of |const| objects, but it should be a
|const| array of |const| objects.  The problem leif encountered was that he had
an array from a |const| object, so it was not a viable candidate in gcc's eyes,
because |SetAttributes| required a non-|const| array of |const| objects.

Comment 2

[John Bandhauer]

I agree. We were close to having it right :(
I wonder how much damage will be done by fixing this.
Reassigning to xpidl owner -> [e-mail]

Comment 3

[David Bradley]

I'll make a patch to xpidl and see what breaks. jband, scc, are they any other 
constructs that might have similar problems? I can't think of anything off the 
top of my head.

Comment 4

[John Bandhauer]

This should be a problem for 'in' arrays of pointer type things... nsID? 
wstring? *not* interfaces because we don't want them to be const.

See the place where it decides to use 'const' in write_param. Apply the same 
sort of test to adding an additional 'const' in the place where it adds an 
additional '*' for arrays in write_param. It looks like domstring *won't* be 
included in this subtest here since it is a 'dipper' type and would fail the 
initial test of whether or not an extra '*' was needed anyway.

Comment 5

[David Bradley]

This is not good. I should of thought of this before. To change the generated 
header declaration all the implementations would have to change which in turn 
might require further fixes to code that relies on the non-const of the 
parameter. It's the right thing to do but doesn't seem like a an easy job.

Comment 6

[David Bradley]

Is the effort involved to fix all the places that would break worth trying to
get this in before 1.0?

Comment 7

[David Bradley]

After further discussion it was decided 1.0.1 makes more sense as a post 1.0
milestone.

Comment 8

[David Bradley]

Retargetting to the proper post 1.0 milestone

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Since const often allows compilers to make optimizations that it otherwise
couldn't make, should this bug have the perf keyword?

Comment 10

[David Bradley]

Moving out to 1.3. If this needs to be in before 1.3 please comment.

Comment 11

[David Bradley]

Moving to 1.4 Alpha

Comment 12

[David Bradley]

Moving out

Comment 13

[David Bradley]

Moving out

Comment 14

[David Bradley]

Moving out

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

This sounds like it would be more doable now that we have rewriting tools.  CCing Taras for his thoughts...

Comment 16

[(dormant account)]

Yeah, I'm pretty sure I could do this. Is this landable in 3.1, I assume it would break source compatibility for some things? Is the exact problem that all of manually declared derivatives of the interface need to go from "const char **" to "const char *const*"?

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

I suspect this is a trunk sort of thing more than a 1.9.1 sort of thing, but you'd have to check with drivers to be sure.  The exact problem is what you describe, in addition to dealing with any code that depends on the current signatures, as per comment 5.

Comment 18

[Brendan Eich [:brendan]]

This is post-1.9.1 for sure.

/be