Experimental Physics and
Industrial Control System

Michael Davidsaver via Core-talk <core-talk at aps.anl.gov> · Fri, 12 Mar 2021 09:12:41 -0800

On 3/12/21 8:53 AM, Ralph Lange via Core-talk wrote:
> On Fri, 12 Mar 2021 at 17:29, Michael Davidsaver <mdavidsaver at gmail.com <mailto:mdavidsaver at gmail.com>> wrote:
> 
>     On 3/12/21 8:05 AM, Ralph Lange via Core-talk wrote:
>     > On Fri, 12 Mar 2021 at 16:06, Michael Davidsaver <mdavidsaver at gmail.com <mailto:mdavidsaver at gmail.com> <mailto:mdavidsaver at gmail.com <mailto:mdavidsaver at gmail.com>>> wrote:
>     >
>     >     On 3/12/21 7:04 AM, Ralph Lange via Core-talk wrote:
>     >     > On Fri, 12 Mar 2021 at 16:01, Michael Davidsaver <mdavidsaver at gmail.com <mailto:mdavidsaver at gmail.com> <mailto:mdavidsaver at gmail.com <mailto:mdavidsaver at gmail.com>> <mailto:mdavidsaver at gmail.com <mailto:mdavidsaver at gmail.com> <mailto:mdavidsaver at gmail.com <mailto:mdavidsaver at gmail.com>>>> wrote:
>     >     >
>     >     >     On 3/12/21 12:07 AM, Ralph Lange via Core-talk wrote:
>     >     >     > In Base 7.0.5, which is a minor release, interfaces have been removed in some of the PVA modules:
>     >     >     >
>     >     >     > pvDatabaseCPP release 4.5.3
>     >     >     >
>     >     >     >     The previously deprecated destroy methods have been removed.
>     >     >     >
>     >     >     >
>     >     >     > pvDataCPP release 8.0.4 (claimed) / 8.1.0 (internal)
>     >     >     >
>     >     >     >     Incompatible changes
>     >     >     >
>     >     >     >         Remove ByteBuffer::align()
>     >     >     >
>     >     >     >
>     >     >     > I did not expect backward compatibility breaking code changes in a minor release.
>     >     >     > Can someone please clarify for me what changes are accepted for which level of release jump?!
>     >     >
>     >     >     wrt. ByteBuffer::align() was unused, and I guess I could have added that this method
>     >     >     was broken.  So in addition to being unused it was also unusable.
>     >     >
>     >     >
>     >     > So - should I just remove the call?
>     >
>     >     Remove from?  pvDataCPP or some ITER code?
>     >
>     >
>     > ITER code, of course. The EPICS modules you have taken care of, I guess.
> 
>     Ah, then you have the larger problem of potentially being subject to the broken-ness.
> 
>     Since I know you are under a time constraint, in the short term I would suggest reverting
>     07b79693af66812c02044c36f1eb2e7cc3cabe8c locally.
> 
>     The larger issue is that ByteBuffer::align() is computing based on the alignment of
>     the char* internal to the ByteBuffer, as opposed to the start of serialization.
>     Since PVD supports serialization in multiple parts at the discretion of the sender,
>     this opens the possibility of unexpected padding being added to longer messages.
> 
> 
> Got it.
> Since we are building Base converted into a CODAC module from our Subversion repository with ~75 downstream modules depending on it, reverting a specific upstream git commit is pretty far from being a trivial and quick operation.
> Is there any up-to-date doc on how user code should handle alignment nowadays? Or a piece of code that shows it?

No.  PVA encoding doesn't preserve alignment [1].  ByteBuffer::align()
was not used internally.  So, there was no explicit or implicit test
coverage, nor other usage examples that I'm aware of.

The best I can think of now is that SerializableControl and
DeserializableControl implementations would need to track the total
number of bytes handled so far (across flushes) and use this to compute
alignment.

[1] https://github.com/epics-base/pvDataCPP/issues/54

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System