llvm dev - Jul 2010 - [LLVMdev] [HEADSUP] Another attempt at CallInst operand rotation

Am 30.06.2010 um 23:31 schrieb John Criswell:
>
> Stupid question: is making the getOperand() method of CallInst  
> going to work?  For example, if I have the following code:
>
> void
> method (Instruction * I) {
>    I->getOperand(2);
>    ...
> }
>
> void method2 (CallInst * CI) {
>    method (CI);
>    ...
> }
>
> Will method() still work when you make CallInst::getOperand() private?
This is the case where I cannot help you (access via baseclass pointer).

For all these cases (there were a few in the llvm codebase too) I got
  - either assertion failures in the test suite
  - or obvious crashes
  - or miscompilations.

To catch all of these I could publish a patch (but not check it in)
that asserts that User::getOperand is not called on a CallInst.
But I can tell you that this will probably give you many false  
positives.

Btw. "method" above is of very little practical value, because
without knowing what type of instruction you have it makes
no sense to get its third operand. You will normally have a
dyn_cast<CallInst>(I) somewhere in "method".

Cheers,

	Gabor

>
> -- John T.
>
>> At this point we will have caught 99% of all low-level clients  
>> out  there.
>>
>> What uncertainties will remain? I can think of two of them:
>>
>>    o getOperandNo()
>>    o access via baseclass pointer
>>
>> The former is a method on Value::use_iterator and I cannot see a  
>> way to
>> intercept it at compile-time.
>> The latter is always possible and does circumvent the above measures,
>> there is no remedy against it.
>>
>> I plan to fire each of these two rounds with one week delay and  
>> monitor
>> the LLVM mailing lists while they are soaking.
>>
>> After that I'll commit the actual operand rotation.
>>
>> Last but not least, there will be some cleanup commits:
>>
>>   - removing CallInst::ArgOffset,
>>   - fixing the 80-column violations I have introduced,
>>   - doxygenizing the new interfaces,
>>   - re-enabling the low-level interface again (possibly
>>     after 2.8 has brached?).
>>
>> Well, that's it. I hope that this order of commits will keep
>> the pain at a bearable level for everyone.
>>
>> I would be thankful for any comments/suggestions
>> regarding this plan.
>>
>> Cheers,
>>
>> 	Gabor
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>

Gabor Greif wrote:
> Am 30.06.2010 um 23:31 schrieb John Criswell:
>
>   
>> Stupid question: is making the getOperand() method of CallInst  
>> going to work?  For example, if I have the following code:
>>
>> void
>> method (Instruction * I) {
>>    I->getOperand(2);
>>    ...
>> }
>>
>> void method2 (CallInst * CI) {
>>    method (CI);
>>    ...
>> }
>>
>> Will method() still work when you make CallInst::getOperand() private?
>>     
>
> This is the case where I cannot help you (access via baseclass pointer).
>   

> For all these cases (there were a few in the llvm codebase too) I got
>   - either assertion failures in the test suite
>   - or obvious crashes
>   - or miscompilations.
>
> To catch all of these I could publish a patch (but not check it in)
> that asserts that User::getOperand is not called on a CallInst.
> But I can tell you that this will probably give you many false  
> positives.
>
> Btw. "method" above is of very little practical value, because
> without knowing what type of instruction you have it makes
> no sense to get its third operand. You will normally have a
> dyn_cast<CallInst>(I) somewhere in "method".
>   
It's example code to illustrate my concern.  It doesn't exist anywhere.
Perhaps a more realistic example would be:

void
method (Instruction * I) {
    for (unsigned index = 0; index < I->getNumOperands(); ++index) {
       do_something_with (I->getOperand(index));
    }
}

For example, code that computes a static backwards slice might do 
something like the above.  It doesn't care about operand order or what 
type of instruction it is dealing with; it just wants to get the 
incoming operands and do something with them.

This seems like a reasonable thing to do.  Will code like this work if 
you make CallInst::getOperand() private?  If not, does your patch remove 
code like the above from the LLVM source tree, and with what code do you 
replace it?

-- John T.
> Cheers,
>
> 	Gabor
>
>
>   
>> -- John T.
>>
>>     
>>> At this point we will have caught 99% of all low-level clients  
>>> out  there.
>>>
>>> What uncertainties will remain? I can think of two of them:
>>>
>>>    o getOperandNo()
>>>    o access via baseclass pointer
>>>
>>> The former is a method on Value::use_iterator and I cannot see a  
>>> way to
>>> intercept it at compile-time.
>>> The latter is always possible and does circumvent the above
measures,
>>> there is no remedy against it.
>>>
>>> I plan to fire each of these two rounds with one week delay and  
>>> monitor
>>> the LLVM mailing lists while they are soaking.
>>>
>>> After that I'll commit the actual operand rotation.
>>>
>>> Last but not least, there will be some cleanup commits:
>>>
>>>   - removing CallInst::ArgOffset,
>>>   - fixing the 80-column violations I have introduced,
>>>   - doxygenizing the new interfaces,
>>>   - re-enabling the low-level interface again (possibly
>>>     after 2.8 has brached?).
>>>
>>> Well, that's it. I hope that this order of commits will keep
>>> the pain at a bearable level for everyone.
>>>
>>> I would be thankful for any comments/suggestions
>>> regarding this plan.
>>>
>>> Cheers,
>>>
>>> 	Gabor
>>>
>>>
>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>
>>>       
>
>

Am 01.07.2010 um 17:28 schrieb John Criswell:
> Gabor Greif wrote:
>> Am 30.06.2010 um 23:31 schrieb John Criswell:
>>
>>
>>> Stupid question: is making the getOperand() method of CallInst   
>>> going to work?  For example, if I have the following code:
>>>
>>> void
>>> method (Instruction * I) {
>>>    I->getOperand(2);
>>>    ...
>>> }
>>>
>>> void method2 (CallInst * CI) {
>>>    method (CI);
>>>    ...
>>> }
>>>
>>> Will method() still work when you make CallInst::getOperand()  
>>> private?
>>>
>>
>> This is the case where I cannot help you (access via baseclass  
>> pointer).
>>
>
>
>
>> For all these cases (there were a few in the llvm codebase too) I got
>>   - either assertion failures in the test suite
>>   - or obvious crashes
>>   - or miscompilations.
>>
>> To catch all of these I could publish a patch (but not check it in)
>> that asserts that User::getOperand is not called on a CallInst.
>> But I can tell you that this will probably give you many false   
>> positives.
>>
>> Btw. "method" above is of very little practical value,
because
>> without knowing what type of instruction you have it makes
>> no sense to get its third operand. You will normally have a
>> dyn_cast<CallInst>(I) somewhere in "method".
>>
>
> It's example code to illustrate my concern.  It doesn't exist  
> anywhere.  Perhaps a more realistic example would be:
>
> void
> method (Instruction * I) {
>    for (unsigned index = 0; index < I->getNumOperands(); ++index) {
>       do_something_with (I->getOperand(index));
>    }
> }
>
> For example, code that computes a static backwards slice might do  
> something like the above.  It doesn't care about operand order or  
> what type of instruction it is dealing with; it just wants to get  
> the incoming operands and do something with them.
>
> This seems like a reasonable thing to do.  Will code like this work  
> if you make CallInst::getOperand() private?  If not, does your  
> patch remove code like the above from the LLVM source tree, and  
> with what code do you replace it?
This code will continue to compile and work as expected.
I'll only trap problematic usage when calling through
CallInst pointer (or reference).

As long as you do not make assumptions in your code
about operand ordering *inside of CallInst* you will be
on the safe side with my upcoming changes. Your above
example is perfectly OK and won't need to be redone.

Hope this helps!

Cheers,

	Gabor

>
> -- John T.
>
>> Cheers,
>>
>> 	Gabor
>>
>>
>>
>>> -- John T.
>>>
>>>
>>>> At this point we will have caught 99% of all low-level clients
>>>> out  there.
>>>>
>>>> What uncertainties will remain? I can think of two of them:
>>>>
>>>>    o getOperandNo()
>>>>    o access via baseclass pointer
>>>>
>>>> The former is a method on Value::use_iterator and I cannot see
>>>> a  way to
>>>> intercept it at compile-time.
>>>> The latter is always possible and does circumvent the above  
>>>> measures,
>>>> there is no remedy against it.
>>>>
>>>> I plan to fire each of these two rounds with one week delay and
>>>> monitor
>>>> the LLVM mailing lists while they are soaking.
>>>>
>>>> After that I'll commit the actual operand rotation.
>>>>
>>>> Last but not least, there will be some cleanup commits:
>>>>
>>>>   - removing CallInst::ArgOffset,
>>>>   - fixing the 80-column violations I have introduced,
>>>>   - doxygenizing the new interfaces,
>>>>   - re-enabling the low-level interface again (possibly
>>>>     after 2.8 has brached?).
>>>>
>>>> Well, that's it. I hope that this order of commits will
keep
>>>> the pain at a bearable level for everyone.
>>>>
>>>> I would be thankful for any comments/suggestions
>>>> regarding this plan.
>>>>
>>>> Cheers,
>>>>
>>>> 	Gabor
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>
>>>>
>>
>>
>