Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CF should not depend on ENDIAN macro (_EL/_EB) #61

Closed
jphickey opened this issue Nov 23, 2021 · 2 comments · Fixed by #160
Closed

CF should not depend on ENDIAN macro (_EL/_EB) #61

jphickey opened this issue Nov 23, 2021 · 2 comments · Fixed by #160
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

This issue was imported from the GSFC issue tracking system

Imported from: [GSFCCFS-1798] CF should not depend on ENDIAN macro (_EL/_EB)
Originally submitted by: Hickey, Joseph P. (GSFC-582.0)[VANTAGE SYSTEMS INC] on Wed Nov 17 09:52:19 2021

Original Description:
It is fairly trivial to write code that is endian-agnostic, or to check at runtime whether the processor should run these copy loops from high-to-low or low-to-high addresses. For example, in CF_MemcpyToBE:

#if ENDIAN == _EL
    dst += (dst_size - 1);
    while (dst_size--)
        *dst-- = *src++;
#elif ENDIAN == _EB
    src += (src_size - dst_size);
    while (dst_size--)
        *dst++ = *src++;
#else

This could be replaced by a simple loop with value shifts rather than making assumptions about memory byte ordering. This would be more correct and portable.

The problem with any #ifdef blocks is that the disabled half of the block never gets executed or tested during the integration, until its ported to some other platform that requires that other block. There is also no guarantee or check that the EL/EB branch actually do the same thing as prescribed, they can diverge.

@jphickey
Copy link
Contributor Author

Imported comment by internal user on Wed Nov 17 11:02:36 2021

Also worth noting that the "CF_CRC_Digest" function has larger chunks of code inside ENDIAN-specific blocks, and furthermore is combined with CF_HW_ALIGNMENT/CF_SW_ALIGNMENT and not all combinations are supported. This is a significant portability issue.

@jphickey
Copy link
Contributor Author

jphickey commented Jan 6, 2022

After PR #137 (which removes a bunch of endian dependencies) the following still remain here:

  • command processing in CF_CmdGetSetParam (3 places)
  • CRC calculation in CF_CRC_Digest

Should be pretty easy to remove at this point.

jphickey added a commit to jphickey/CF that referenced this issue Jan 11, 2022
Removes the "optimization" for big endian, unlikely to be useful.
jphickey added a commit to jphickey/CF that referenced this issue Jan 11, 2022
Removes the "optimization" for big endian, unlikely to be useful.
astrogeco added a commit that referenced this issue Jan 12, 2022
Fix #61, remove dependence on ENDIAN macro for checksum
@skliper skliper added this to the Draco milestone Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants