[Patch] fix IDL hang V2

[Noel Power]

Hi Jelmer,
On 22/08/14 00:12, Jelmer Vernooij wrote:
> On Thu, Aug 21, 2014 at 08:52:11PM +0100, Noel Power wrote:
>> On 21/08/14 17:34, Jelmer Vernooij wrote:
[...]
>>> I'm wary of supporting constructions that are pidl-specific. We'd like to
>>> make our IDL files more compatible with midl, not less.
>> That's not at all clear from the current idl that seems littered with
>> pidl specific extensions. The fact that those specific properties aid
>> the the generation of standard marshall/unmarshall routines is imho a
>> *good* thing and should be encouraged, hand coding marshall/unmarshall
>> routines should be discouraged.
> There are a couple of IDL files that have a lot of custom pidl
> properties, but for the others it's fairly rare.
didn't seem that way to me

find librpc/idl/ -name "*.idl" | wc -l
74
find . -name "*.idl" -type f -print0 | xargs -0 grep public  | cut -f1
-d: | sort -u | wc -l
47
and for the pidl specifics mentioned in the pidl documentation (noprint,
value, relative, subcontext, flag, nodiscriminant, charset)
17, 36, 10, 18, 48, 23, 45

ok, not the most accurate numbers but I would say at least indicative
(and a quick manual look at inline arrays usages also is informative)

>
> Most of the pidl extensions have equivalent attributes in midl. We'd
> like to gradually move over from the pidl-specific attributes to the
> midl specific ones, so we can more freely exchange IDL files.
and that probably makes perfect sense for idl files that are
wanted/needed/desirable to be exchanged, but it would be unfortunate to
bunch all idl files into the same category, after my admittedly short
exposure to pidl I really believe that it would be a shame to limit it's
usefulness and functionality to being a midl clone. It's possible to
have the best of both worlds.
>
> pidl extensions also generally don't translate well to wireshark
> DCE/RPC calls, making it harder or impossible to generate wireshark dissectors
> from IDL files that use pidl extensions.
>
>>> Please add tests for new features (see t/ under the pidl/ directory).
>> is it a new feature to prevent a hang? not at all sure how to write a
>> test for a hang (note: when it hangs on my machine it quickly makes the
>> machine unusable) :-/ but I can try to write a test to see if the
>> compile succeeds if that helps...
> With your change, presumably it would no longer hang? If the test hangs when
> the code is broken, that is not ideal but acceptable.
ok, I will try it (although I know nearly as much about perl as pidl/midl)
>
>> not sure what you are getting at with the above, the structure above (if
>> converted into idl )
>>
>> interface hang
>> {
>> typedef [public] struct {
>> bar a;
>> hyper t;
>> } foo ;
>>
>> typedef [public] struct {
>> foo b;
>> } bar;
>> }
>>
>>
>> will hang if compiled with pidl without the patch, with patched pidl it
>> will succeed but it will result in generated code that rightly won't
>> compile "error: field ‘a’ has incomplete type", however using a pointer
>>
>> interface hang
>> {
>> typedef [public] struct {
>> bar *a;
>> hyper t;
>> } foo ;
>>
>> typedef [public] struct {
>> foo b;
>> } bar;
>> }
>>
>> and the generated code is compilable, I don't see the problem.
> With pointers, why do we need this hack (which is my main concern
> with this patch) at all?
ok, granted the example (non-pointer) is avoidable, but.. the reason why
I came up with that example in the first place was as a result of trying
to reduce a much much more complex interaction of structures that was
causing the hang to the smallest example, the more complex example had
amongst other things an array of the nested element that I reduced to a
single element.

>
> If there's a struct member that is a pointer, then
> can_contain_deferred should never check whether that member itself can
> contain a deferred at all. How does a loop occur here?
I don't believe that I said it did (although in fairness I did think it
might), but really I just was trying to understand the point you were
making with the psuedo code you had
>
> FWIW I can compile the second IDL file without problems here, without
> your patch:
sure, so as can I. But....

interface hang
{
    typedef [public] struct {
        uint32 count;
        bar a[count];
        hyper t;
} foo ;

    typedef [public] struct {
        foo b;
    } bar;
}

will also hang (and could be considered maybe a better mininal
representation of the structure I originally has a problem with)
 
so.. maybe the can_contain_deferred should also check for the presence
of an array,
I will look again to see what is happening

thanks!

Noel