TG-1877: Include array pointer types in needed classes

[smowton]

Lazy methods attempts a pointer graph walk starting from the main function's
parameters to determine what classes may exist before the main function is
entered; for example, starting at f(A a) implies that an A instance may exist,
and if A itself has a B field that too might exist, and so on. However, until
now this failed to account for array-typed parameters and fields. This commit
amends gather_field_types to account for this special case.

Corresponding PR: https://github.com/diffblue/test-gen/pull/1289

[thk123]

I don't think the test is a good one since test.g being the entry point, doesn't this already imply test needs to be loaded?

Extra tests:

• class that contains an array

[thk123]

Prefer java_types.h java_array_element_type (with probably a check that is is an array).

[thk123]

I realise this isn't from this PR - but is there a better way to skip array primitive pointers as this feels like a fairly loose check.

[smowton]

No, the entry point actually does not imply test being available, as it's a static method (thus no instance of class test is implied)

[thk123]

@smowton OK - in which case, I think the addition of the test where the array is loaded transitively and this should be OK (though I think using java_array_element_type would be good).

[antlechner]

I tested it on some examples and it does not work with generic arrays.
Comparing

public class IntegerArray {

    Integer[] array;

    public static boolean testLess(IntegerArray intArr) {
        int first = intArr.array[0];
        int second = intArr.array[1];
        if (first < second)
            return true;
        else
            return false;
    }

}

and

public class GenericArray<E> {

    E[] array;

    public static boolean testLess(GenericArray<Integer> intArr) {
        int first = intArr.array[0];
        int second = intArr.array[1];
        if (first < second)
            return true;
        else
            return false;
    }

}

IntegerArray.testLess generates a correct test (it previously didn't) while GenericArray.testLess still puts zeros everywhere.

[smowton]

@thk123 changes applied.
@antlechner The generic array case indeed doesn't work yet. At the moment I'm not confident I understand how generics have been done well enough to fix it, but a generics team person should be able to improve on this in short order. Will describe the situation in better detail in a moment.

[smowton]

@thk123 here are the problems I ran into briefly to address the generic-array case raised by @antlechner:

1. The generic specialisation is attached to the pointer, like:

pointer
  * width: 64
  * #reference: 1
  * #java_generic_type: 1
  * #type_variables: 
    0: pointer
        * width: 64
        * #reference: 1
        0: symbol
            * identifier: java::test
            * #base_name: test
  0: symbol
      * identifier: java::container
      * #base_name: container

Would it be better to annotate the symbol, as is done for arrays, since it's the object at the end of the pointer that is specialised, not the pointer itself? As it stands functions like this that walk over the type graph have to retrieve generic information at the pointer stage and pass it into the function that handles structs.

2. This specifies that some type variable has been instantiated with test, but what variable has been instantiated is implied by context. Can I always assume that *ptr is the specialised thing (as in, is the pattern always pointer(#type_variables -> [ actual list ], subtype -> symbol (specialised_classname))? Would it be more robust to specialise a name = variable pair, like (pseudocode) lhs: java::container::T, rhs: pointer(java::test)?

3. The actual type I need to specialise is given:

pointer
  * width: 64
  * #reference: 1
  * #java_generic_parameter: 1
  * #type_variables: 
    0: symbol
        * identifier: java::container::T
  0: symbol
      * identifier: java::java.lang.Object
      * #base_name: java.lang.Object

This is the member type of an array T[]. type_variables, plural, suggests that T has been used somehow, but doesn't seem to specify that it replaces Object? What are the rules for using the implied environment to generate a specialised type here? Is there a function that will take my knowledge implied by context that T == test and give me a concrete type back? Longer term, how about we show T in situe, like #generic_type: pointer: symbol: T, such that substituting T for its actual value is sufficient?

[thk123]

Thanks for the extra tests. @mgudemann might have something to say on the generics being attached to a pointer -let's discuss tomorrow when I'm in the office. The only reason I can see to prefer on the pointer is technically it is associated with the pointer since I can do:

gen g = new gen();
gen<Integer> gi = g;
gen<Float> gf = g;

But I don't know if that is relevant.

[thk123]

Could you create and link to to a jira ticket to avoid this test getting lost and then duplicated.

[antlechner]

Could either link to https://diffblue.atlassian.net/browse/TG-1877 and mention in the ticket that the generics case doesn't work yet, or move TG-1877 to "Ready for QA" and create a new ticket for the generics case.

[antlechner]

Sorry for the delay. I've tested these changes with the example in TG-1877 and all examples in TG-1712. All of them work except for the arrays of Floats and Doubles, but that seems to be an unrelated problem. So for non-generic arrays, this PR looks good!

[antlechner]

What is the reason behind using a separate class here rather than just calling assert(args[0].f() == 1); directly?

[smowton]

Turns out lazy loading always marks classes that make assertions as needed! I'm not sure why, I certainly didn't write that code, but I didn't want to meddle with it for now...

[antlechner]

Could either link to https://diffblue.atlassian.net/browse/TG-1877 and mention in the ticket that the generics case doesn't work yet, or move TG-1877 to "Ready for QA" and create a new ticket for the generics case.

[smowton]

@antlechner added comments in both cases. Will merge this once CI passes.

[antlechner]

@smowton I just talked to @thk123 and he asked for a new ticket to be created for the generics case, as that would make it clearer. So I'll make a new ticket in a bit. Sorry for the confusion. If you don't want to update the comment again, we could always link TG-1877 and the new ticket...

[mgudemann]

@thk123 yes, the idea was to implement this as closely as possible as it is in the JVM bytecode
@smowton yes, one could have used the symbol also for annotation of the generic type, but the original goal was to keep it as close as possible to the JVM internals

@smowton for 3 the java_generic_parameter states that is a generic type variable that has not been instantiated with a real type. The Object entry is because any type variable is bound by an object type, with Object being the most permissive, of couse. (I am also not sure whether we actually do support tighter bounds or still have everything as Object @svorenova ?)

[majakusber]

Yes, as far as I know, all bounds are set to Object currently, the task to parse bounds is TG-1286.

[smowton]

@mgudemann @svorenova it looks like the information is actually already there? Look at the #type_variables with child test, isn't that indicating the concrete type container<test>?

[mgudemann]

@smowton was referring to point 3) where this is not the case. In 1) yes, it is the instantiation of the pointed to type, there is no java_generic_parameter, but a reference type which represented the concrete object type. So in 1) you have a container<test> . In 3) this is the type variable only which is replaced by a reference type when doing the specialisation.

Please correct me if I am wrong @svorenova

[antlechner]

Created a new ticket for the generics case, in case a new PR is needed for that:
https://diffblue.atlassian.net/browse/TG-1938

[majakusber]

@mgudemann Yes, you are correct. I just talked to @smowton and hopefully have pointed him towards the right direction. The problem is, if I understood correctly from the discussion, how to quickly find, given a generic type with concrete types, the corresponding specialized java generic class. There is a function called build_generic_name that does almost exactly what is needed.

[smowton]

@thk123 @antlechner @mgudemann @svorenova this has been updated to support generic type arguments, with a few extra tests too. One fails due to inadequate generic support, but lazy loading appears to do the right thing at least.

[thk123]

I've created a diffblue/test-gen#1317 to check this works, assuming it does I will merge unless anyone has any objections

[thk123]

Unfortunately this creates quite a few failures in the TG repo - so until someone has a had a chance to investigate and fix these can I request this doesn't go in.

[smowton]

I've updated this to work around the vast majority of failures -- the problem was that the string library version of String contains a uint16_t*, which is usually impossible in Java. However it has exposed a generics bug in the unit tests, and broken a few string tests for reasons I don't understand yet. Nontheless the failure count in test-gen is down from hundreds to ~5.

[smowton]

@thk123 fixed the remaining problem, which was an incidentally perturbed string test (see commit message at https://github.com/diffblue/test-gen/pull/1317 for more detail). Now waiting for test-gen CI (and a re-check here).

[smowton]

Passes, ready to merge I think

[thk123]

It'd be good if you could rebase the test-gen PR before merging, but looks fine.

[smowton]

@thk123 I'll rebase both once https://github.com/diffblue/test-gen/pull/1063 lands, thus bringing latest cbmc and latest test-gen back into line.

[smowton]

This is currently blocked because #1419 was merged, but doesn't currently work with test-gen. @peterschrammel is currently debugging that, after which we'll be able to advance the tg pointer and get this in.

[smowton]

Rebased, and rebased @thk123's bump PR; will merge if CI passes both.