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

Static allocation and ringbuf.h build issue - wrong size in static assert. (IDFGH-10479) #11726

Closed
3 tasks done
phelter opened this issue Jun 21, 2023 · 1 comment
Closed
3 tasks done
Assignees
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Type: Bug bugs in IDF

Comments

@phelter
Copy link

phelter commented Jun 21, 2023

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

v5.1-rc1

Operating System used.

Linux

How did you build your project?

Command line with CMake

If you are using Windows, please specify command line type.

None

What is the expected behavior?

When SUPPORT_STATIC_ALLOCATION=1:

The esp_ringbuf/include/freertos/ringbuf.h definition of StaticRingbuffer_t does not match the Ringbuffer_t definition when it comes to size. So when compiling with GNU the assertion here fails.

What is the actual behavior?

Asserts the static assertion when building with STATIC_ALLOCATION - here

Steps to reproduce.

Not providing reproduce steps that I used to find the issue, as I am building natively (linux) to mock out the ESP-IDF environment and consuming the headers inside the mocks for googletest and this is where the issue is identified.

Possible steps to reproduce.

  1. Enable SUPPORT_STATIC_ALLOCATION
  2. Build a main that uses the ringbuf.h with static allocation with GNU compiler newer than 4.6.

Build or installation Logs.

esp_idf-src/components/esp_ringbuf/ringbuf.c:77:1: error: static assertion failed: "StaticRingbuffer_t != Ringbuffer_t"
   77 | _Static_assert(sizeof(StaticRingbuffer_t) == sizeof(Ringbuffer_t), "StaticRingbuffer_t != Ringbuffer_t");
      | ^~~~~~~~~~~~~~

More Information.

This is fixed when the definition of the StaticRingbuffer_t is changed to:

#if (configSUPPORT_STATIC_ALLOCATION == 1)
typedef struct xSTATIC_RINGBUFFER
{
    /** @cond */ //Doxygen command to hide this structure from API Reference
    size_t      xDummy1[2];
    UBaseType_t uxDummy2;
    void       *pvDummy3[11]; // Note swap here must be in same order in case of padding.
    BaseType_t  xDummy4;
    // StaticSemaphore_t xDummy5[2];
    List_t       xDummy5[2]; // TaskWaitingtosend/receive
    void        *xDummy6;    // QueueSet
    portMUX_TYPE muxDummy;
    /** @endcond */
} StaticRingbuffer_t;
#endif

Also please add:

#if (configSUPPORT_STATIC_ALLOCATION == 1)
RingbufHandle_t xRingbufferCreateStatic(size_t xBufferSize, 
                                        RingbufferType_t xBufferType, 
                                        uint8_t *pucRingbufferStorage,
                                        StaticRingbuffer_t *pxStaticRingbuffer);
#endif

check for SUPPORT_STATIC_ALLOCATION as the xRingbufferCreateStatic OR add some build level check that it is not used when SUPPORT_STATIC_ALLOCATION = 0.

@phelter phelter added the Type: Bug bugs in IDF label Jun 21, 2023
@espressif-bot espressif-bot added the Status: Opened Issue is new label Jun 21, 2023
@github-actions github-actions bot changed the title Static allocation and ringbuf.h build issue - wrong size in static assert. Static allocation and ringbuf.h build issue - wrong size in static assert. (IDFGH-10479) Jun 21, 2023
@Dazza0 Dazza0 self-assigned this Jun 22, 2023
@Dazza0
Copy link
Contributor

Dazza0 commented Jun 22, 2023

@phelter

Thanks for catching this. Fix is pending merge on master and backport to v5.1.

@espressif-bot espressif-bot assigned Dazza0 and unassigned Dazza0 Jun 22, 2023
@espressif-bot espressif-bot added Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed labels Jun 22, 2023
espressif-bot pushed a commit that referenced this issue Jul 7, 2023
When building on linux/host compilers (e.g., GCC), the compiler may add padding
depending on the size and order of the member types.

This commit fixes the ordering or the StaticRingbuffer_t such that it matches
the internal Ringbuffer_t. The "_Static_assert" is always enabled for all
compilers.

Closes #11726
espressif-bot pushed a commit that referenced this issue Jul 19, 2023
When building on linux/host compilers (e.g., GCC), the compiler may add padding
depending on the size and order of the member types.

This commit fixes the ordering or the StaticRingbuffer_t such that it matches
the internal Ringbuffer_t. The "_Static_assert" is always enabled for all
compilers.

Note: Removed all usage configSUPPORT_STATIC_ALLOCATION preprocessor conditions
as the macro is always set to 1.

Closes #11726
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Type: Bug bugs in IDF
Projects
None yet
Development

No branches or pull requests

3 participants