Compiler warnings arising from use of stack-allocated temporary FB instances

[DavidHopkinsFbr]

Hi,

When I build projects that use TcMatrix I often get a fair few warnings like these:

I've seen these in TwinCAT 3.1.4024.17, .29, and .44.

In case the image doesn't upload properly, here's a text version:

Severity	Code	Description	Project	File	Line	Suppression State
Warning		Compatibility warning: FB_Exit is now called in Vector_CrossProduct for FB instance M_Vectors which is located on the stack. This has not been the case for code compiled with compiler versions < 3.1.4022.0. [tcmatrix, 1.4.3 (burks engineering)]		Vector_CrossProduct	0	
Warning		Compatibility warning: FB_Exit is now called in Grow for FB instance Temp which is located on the stack. This has not been the case for code compiled with compiler versions < 3.1.4022.0. [tcmatrix, 1.4.3 (burks engineering)]		DynamicMatrix.Grow	0	
Warning		Compatibility warning: FB_Exit is now called in Shrink for FB instance Temp which is located on the stack. This has not been the case for code compiled with compiler versions < 3.1.4022.0. [tcmatrix, 1.4.3 (burks engineering)]		DynamicMatrix.Shrink	0	
Warning		Compatibility warning: FB_Exit is now called in Transpose for FB instance Temp which is located on the stack. This has not been the case for code compiled with compiler versions < 3.1.4022.0. [tcmatrix, 1.4.3 (burks engineering)]		DynamicMatrix.Transpose	0	
Warning		Compatibility warning: FB_Exit is now called in TransposeSquare for FB instance Temp which is located on the stack. This has not been the case for code compiled with compiler versions < 3.1.4022.0. [tcmatrix, 1.4.3 (burks engineering)]		Matrix.TransposeSquare	0	

The issue appears to arise from a new compiler warning added to CoDeSys in V3.5.9.70. (You are probably aware that the TwinCAT PLC module is based on CoDeSys. New CoDeSys features and bug fixes generally show up in a new TwinCAT release after a while....) Luckily for us, CoDeSys actually publishes release notes, unlike Beckhoff.

https://www.codesys.com/fileadmin/data/Images/System/Releaseinformation/ReleaseNote-CODESYS-V35970.pdf

FB_Exit should be called to destroy local instances CDS-383
FB_EXIT is now called for instances allocated on the stack before the owning scope returns.
A warning has been added to inform users about the change of semantics in these cases.
In order to suppress the warning, decorate the POU declaring the local instance with the warning disable
macro for the warning code C0394 ("{warning disable C0394}")

Basically, this {warning disable C0394} attribute needs to be attached to the Function or Method or Property getter/setter which allocates the temporary FB.

I have tested that the warning suppression works on my machine; it does. I can submit a PR if it would be helpful. I haven't been through the whole library yet to locate every POU which allocates an FB instance on the stack, though.

However I'm less confident in my belief that suppressing the warning is the correct thing to do in all cases. Do you think there are any situations in TcMatrix (or TcTransform) where calling FB_Exit() for temporary stack-allocated FB instances will cause problems? What's your minimum TwinCAT support version? If it's less than 4022.0, are code changes necessary to make sure that those stack-allocated FBs are cleaned up properly in the absence of a compiler-enforced call to FB_Exit()?

[DavidHopkinsFbr]

I have to admit that I am a bit hazy on exactly what are the performance implications of allocating FB instances as temporary stack variables.

The Beckhoff TwinCAT PLC static analysis tool can be configured to complain about FB stack allocations, and is configured that way by default. It's a pretty good heuristic most of the time, because most FBs are stateful and reallocating them on each cyclic call would make them lose their state (which would be a bug for most FB usages).

But the FBs used in these methods don't need to retain anything beyond the duration of the call, so it's fine from a correctness perspective; I'm just not sure whether allocating and deallocating FB instances has substantially more overhead than a STRUCT of similar total size, to a degree that would significantly affect CPU load of a project.

I suppose I should just measure it....

[BurksEngineering]

Thanks for bringing this up! I was unclear on the details of that compiler warning, but figured I could ignore it because the unit tests were passing 😬

I think this affects quite a few methods, because I like to use VAR allocation in methods (instead of pre-allocating in the base FB) to encapsulate the logic. You make a good point on performance though. I originally thought about making a 'temp' matrix inside every matrix just for use in whatever method might need it (like I did with Ri, Ci, and I indexers to avoid having to re-declare them constantly) but I realized this would end up making an infinite chain of recursive matrix FBs in some cases! My compromise was to put as much logic as I could into external helper functions that do not have 'state', so that people who are worried about performance or state can use those functions directly.

Going through and auditing all of the methods before slapping the warning disable attribute on them sounds like a smart idea (I didn't even know how to make that attribute work, so thank you for sharing the snippet!). Just on a cursory glance I see some issues worth cleaning up: in Matrix.TransposeSquare() there is a VAR DynamicMatrix created but never used! However for methods like DynamicMatrix.Grow()/Shrink() I think the use of the Temp matrix is necessary (although I'm sure a more efficient algorithm could be used in these cases).

Thanks for submitting this issue and feel free to submit a PR for any methods you think could get the warning-suppression-attribute slapped on them, even if it isn't a comprehensive audit of the whole library. I'm not going to prioritize adding the attributes right now but I'll leave the issue open as a reminder for next time I make a change.