[v2 1/2] d3dx9: Support relative addressing in preshader.

[Matteo Bruni]

2017-04-03 19:50 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> On 04/03/2017 06:35 PM, Matteo Bruni wrote:
>>
>> 2017-03-29 20:17 GMT+02:00 Matteo Bruni <matteo.mystral at gmail.com>:
>>>
>>> Yeah and temporaries are supposed to be written before being read so
>>> in theory you can entirely skip the update_table_size() calls for
>>> source arguments.
>>
>> I haven't looked in detail but this still works for me (i.e.
>> commenting out those calls doesn't break the tests here, on 32 bits at
>> least). How does the accidental CTAB overwriting you mention happen
>> exactly?
>
>     Yes, if to just comment out it won't break any test, it will break that
> one though if you replace table update with input operand offset range check
> with failure return (after moving table size update from input parameters
> above the loop of course, otherwise it will break many tests).
>     I actually spotted the accidental CTAB overwrite before that by watching
> warn/trace output of tests and investigating why there are 'uninitialized
> input' warnings on preshader operations testing, and had in mind to fix this
> test at some point. Overwriting happens in test_preshader_op() function for
> 2 argumemts op case. The function thinks there are 4 per-component
> operations (which is the case in test blob for 1 argument test preshader but
> actually not the case for 2 argument preshader blob), and puts opcode there,
> while the last opcode fall after the actual end of preshader code to the
> CTAB of the next preshader.
>     I agree that this part with table size update based on input operands
> actual offset looks a bit ugly and can be most likely made better. I just
> suspect that the better solution which won't break anything that can work
> now same as native needs a bit more investigation of how native code handles
> out of bound access in this case. Just dropping it without failing will lead
> to crash or undefined behaviour if index register's offset is out of bound
> now (which should not normally happen in my current understanding though
> without broken bytecode, as well as the need to update table size based on
> input operands as well). This of course can be mitigated by also checking
> index register offsets at runtime (now the check done for value register
> offsets only which can now include index register value), but I am not sure
> that moving this extra check to execution time is much nicer. So maybe this
> improvement can wait until we get all the remaining major pieces in place
> (that pieces are, in my understanding, are state manager support and effect
> pools/shared parameters, which make d3dx9 effect framework usable with the
> majority of applications)? Or a simple solution is to fix the test and add
> the checks instead of table update, ignoring compatibility with native
> behaviour for such cases, which we consider possible on broken bytecode
> only, at least based on what I've tested so far.

I see. It's okay to handle this as native and be robust to
inconsistent data in constant tables.

>>> Hmm, I've never seen any effect with a number of immediates not
>>> multiple of 4. Can you point me to one?
>>
>> Do you have one of those?
>
>     I somehow was sure that there are such cases in our present tests,
> though could not find them now. I pretty sure I saw the values of '6' as
> immediate constant table size, maybe they were present in some previous
> versions of blobs when there was less immediate constants used in relative
> addressing test, or during my extensive testing of out of bounds indexes.
> Anyway, I found such a case (18 immediate constants) in one of real apps I
> am also testing this on, I am attaching a text file with that preshader dump
> (it also has an immediate constants count display which I temporary added to
> the preshader dump code, the same way you do in your patch, without the
> values dump though).

It looks to me as the CLIT section in the bytecode you attached
contains 16 as the constants count. Not sure why the print below
doesn't match that.

Either way, it doesn't matter much. Even if immediate counts
non-multiple-of-4 can actually exist we could (and I argue, should)
bump that up to the next multiple, zeroing the extra components.