Readability enhancements in heap_1.c

[wdfk-prog]

Readability enhancements in heap_1.c

Use the macro heapADD_WILL_OVERFLOW as in other heap implementations to make the code more readable. These changes were introduced to other heaps in this PR.

Test Steps

Built the Posix_GCC demo with heap_1.c.

Checklist:

• [x] I have tested my changes. No regression in existing tests.
• [NA] I have modified and/or added unit-tests to cover the code changes in this Pull Request.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[aggarg]

if( ( xWantedSize + ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) ) ) > xWantedSize )
{
    xWantedSize += ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) );
}
else
{
    xWantedSize = 0;
}

The correct way to think about the above code is that you are adding a value to xWantedSize which could result in overflow:

xAdditionalRequiredSize = portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK );

if( ( xWantedSize + xAdditionalRequiredSize ) > xWantedSize )
{
    xWantedSize += xAdditionalRequiredSize;
}
else
{
    xWantedSize = 0;
}

Therefore, removing the overflow check is not correct. However, we can certainly enhance readability here. Let me get a patch for that.

[aggarg]

Removing configADJUSTED_HEAP_SIZE is also not correct because we may not start at the &( ucHeap[ 0 ] ) if that is not aligned properly:

if( pucAlignedHeap == NULL )
        {
            /* Ensure the heap starts on a correctly aligned boundary. */
            pucAlignedHeap = ( uint8_t * ) ( ( ( portPOINTER_SIZE_TYPE ) &( ucHeap[ portBYTE_ALIGNMENT - 1 ] ) ) &
                                             ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) );
        }

We need to ensure that we are not allocating memory that is not part of ucHeap and configADJUSTED_HEAP_SIZE is needed for that.

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[wdfk-prog]

if( ( xWantedSize + xAdditionalRequiredSize ) > xWantedSize )
{
    xWantedSize += xAdditionalRequiredSize;
}
else
{
    xWantedSize = 0;
}

I still don't understand. Can you give me an example? If xWantedSize is an unsigned integer, as I understand it, this is always true

[wdfk-prog]

Removing configADJUSTED_HEAP_SIZE is also not correct because we may not start at the &( ucHeap[ 0 ] ) if that is not aligned properly:

if( pucAlignedHeap == NULL )
        {
            /* Ensure the heap starts on a correctly aligned boundary. */
            pucAlignedHeap = ( uint8_t * ) ( ( ( portPOINTER_SIZE_TYPE ) &( ucHeap[ portBYTE_ALIGNMENT - 1 ] ) ) &
                                             ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) );
        }

We need to ensure that we are not allocating memory that is not part of ucHeap and configADJUSTED_HEAP_SIZE is needed for that.

I understand this step, if not deducted, pucAlignedHeap may be smaller than the original address after the upward alignment process.But if it's a bit wasteful to directly deduct an alignment size, you can deduct less.

[aggarg]

I still don't understand. Can you give me an example? If xWantedSize is an unsigned integer, as I understand it, this is always true

We are trying to avoid integer overflow here. Integer Overflow - An integer overflow happens when we try to store inside an integer variable a value that is larger than the maximum value the variable can hold.

Take the following example:

xWantedSize = 0xFFFFFFFF;
xAdditionalRequiredSize = 5;

Assuming that size_t is 32-bit, adding the above 2 values will result in an integer overflow. Is it clear?

[aggarg]

if not deducted, pucAlignedHeap may be smaller than the original address

It is worse than that. The heap will end up allocating memory from a memory region which is not reserved for heap. Look at the following diagram-

                configTOTAL_HEAP_SIZE                                 
   <----------------------------------------------------->              
ucHeap
   |
   |
   V
   +------+----------------------------------------------+------+       
   |      |                                              | ---- |       
   |      |                                              | ---- |       
   +------+----------------------------------------------+------+       
          ^                                              <------>       
          |                                          Not part of ucHeap
          |
     pucAlignedHeap                                                  
          <----------------------------------------------------->       
                      configTOTAL_HEAP_SIZE                            

pucAlignedHEAP + configTOTAL_HEAP_SIZE will cover an area of memory shown as "Not part of ucHeap". Hope that clarifies.

[wdfk-prog]

I still don't understand. Can you give me an example? If xWantedSize is an unsigned integer, as I understand it, this is always true

We are trying to avoid integer overflow here. Integer Overflow - An integer overflow happens when we try to store inside an integer variable a value that is larger than the maximum value the variable can hold.

Take the following example:

xWantedSize = 0xFFFFFFFF;
xAdditionalRequiredSize = 5;

Assuming that size_t is 32-bit, adding the above 2 values will result in an integer overflow. Is it clear?

Thanks. I see.

[wdfk-prog]

if not deducted, pucAlignedHeap may be smaller than the original address

It is worse than that. The heap will end up allocating memory from a memory region which is not reserved for heap. Look at the following diagram-

                configTOTAL_HEAP_SIZE                                 
   <----------------------------------------------------->              
ucHeap
   |
   |
   V
   +------+----------------------------------------------+------+       
   |      |                                              | ---- |       
   |      |                                              | ---- |       
   +------+----------------------------------------------+------+       
          ^                                              <------>       
          |                                          Not part of ucHeap
          |
     pucAlignedHeap                                                  
          <----------------------------------------------------->       
                      configTOTAL_HEAP_SIZE                            

pucAlignedHEAP + configTOTAL_HEAP_SIZE will cover an area of memory shown as "Not part of ucHeap". Hope that clarifies.

I can see why need to deduct portBYTE_ALIGNMENT here. Now I think don't need to deduct portBYTE_ALIGNMENT so much, but can deduct less.

for example: The address allocation for ucHeap starts at 0x20000007 portBYTE_ALIGNMENT = 8 In this case,pucAlignedHeap is aligned to 0x0x20000008 the first time it is allocated So configADJUSTED_HEAP_SIZE can be configTOTAL_HEAP_SIZE-1 instead of configTOTAL_HEAP_SIZE-portBYTE_ALIGNMENT

This is my modification. Could you please help to see if it is reasonable?

https://github.com/wdfk-prog/FreeRTOS-Kernel/commit/f928e58b760c19ee3d5e644b97cdbc400889fdfb

[aggarg]

so much, but can deduct less.

Yes, you can save couple of bytes here. The question though is the added complexity worth it? The code you shared had a bug and I added a comment on your commit.

[wdfk-prog]

so much, but can deduct less.

Yes, you can save couple of bytes here. The question though is the added complexity worth it? The code you shared had a bug and I added a comment on your commit.

If think about it that way, then there's really no need to add more code for just a few bytes of space. Thank you for your reply. I have no more questions.