Author Topic: Weird things in vendor library code...  (Read 1466 times)

0 Members and 1 Guest are viewing this topic.

Offline westfwTopic starter

  • Super Contributor
  • ***
  • Posts: 4639
  • Country: us
Weird things in vendor library code...
« on: August 08, 2025, 08:59:48 pm »
We've seen many complaints about STM Cube and Atmel ASF.
Here's another doozy that I found, investigating slow random number generation on Arduino Uno R4... (Renesas RA4M Cortex-M4)

Does anyone have any thoughts one WHY this would be written this way?  The compiled binary doesn't actually have any code to match those statements...  (As you'd expect, with optimization.)  Templates?  Debugging?  Cross reference hacking?
Code: [Select]
fsp_err_t HW_SCE_GenerateRandomNumberSub(uint32_t *OutData_Text)
{
    uint32_t iLoop    = 0U;
    uint32_t iLoop1   = 0U;
    uint32_t iLoop2   = 0U;
    int32_t  jLoop    = 0U;
    uint32_t kLoop    = 0U;
    uint32_t oLoop    = 0U;
    uint32_t oLoop1   = 0U;
    uint32_t oLoop2   = 0U;
    uint32_t dummy    = 0U;
    uint32_t KEY_ADR  = 0U;
    uint32_t OFS_ADR  = 0U;
    uint32_t MAX_CNT2 = 0U;
    (void)iLoop;
    (void)iLoop1;
    (void)iLoop2;
    (void)jLoop;
    (void)kLoop;
    (void)oLoop;
    (void)oLoop1;
    (void)oLoop2;
    (void)dummy;
    (void)KEY_ADR;
    (void)OFS_ADR;
    (void)MAX_CNT2;
    if (0x0U != (SCE->REG_1BCH & 0x1fU))
    {
        return FSP_ERR_CRYPTO_SCE_RESOURCE_CONFLICT;


It also has several of these:
Code: [Select]
SCE->REG_100H = change_endian_long(0x00000000U);
(Can't have that zero with the wrong endianness!  This in fact produces a function call!)
https://github.com/renesas/fsp/blob/master/ra/fsp/src/r_sce/crypto_procedures/src/sce5/plainkey/primitive/hw_sce_p09.c#L39
« Last Edit: August 08, 2025, 11:40:31 pm by westfw »
 

Offline westfwTopic starter

  • Super Contributor
  • ***
  • Posts: 4639
  • Country: us
Re: Weird things in vendor library code...
« Reply #1 on: August 08, 2025, 09:09:07 pm »
PS: Renesas doesn't document the "Secure Cryptographic Engine", even to the extent needed to access the random number generator.   The source code is no help whatsoever...  Grr.
Code: [Select]
    SCE->REG_84H = 0x00000901U;    SCE->REG_108H = 0x00000000U;
    HW_SCE_func001(0x3f063139U, 0xcb903ce7U, 0x4c5616a7U, 0x4970827aU);
    SCE->REG_104H = 0x00000251U;
    SCE->REG_A4H = 0x0020363cU;
 

Offline DiTBho

  • Super Contributor
  • ***
  • Posts: 5083
  • Country: gb
Re: Weird things in vendor library code...
« Reply #2 on: August 08, 2025, 09:33:28 pm »
Renesas

dunno, but I usually stay away from Renesas.
Poor documentation.
Weird code.
The opposite of courage is not cowardice, it is conformity. Even a dead fish can go with the flow
 

Offline ataradov

  • Super Contributor
  • ***
  • Posts: 12463
  • Country: us
    • Personal site
Re: Weird things in vendor library code...
« Reply #3 on: August 08, 2025, 10:17:46 pm »
Likely generated code and they could not be bothered to come up with good templates.

I had to write templated code for Harmony, and getting a good output when a lot of things can be enabled or disabled is not easy. Some people don't care enough to even try.
Alex
 

Offline gerbay

  • Regular Contributor
  • *
  • Posts: 85
  • Country: tr
Re: Weird things in vendor library code...
« Reply #4 on: August 08, 2025, 10:27:38 pm »
There are variables defined but not used. Many compilers generates "variable defined but not used" warning.

below lines eliminates "variable defined but not used" warning.

Code: [Select]
    (void)iLoop;
    (void)iLoop1;
    (void)iLoop2;
    (void)jLoop;
    (void)kLoop;
    (void)oLoop;
    (void)oLoop1;
    (void)oLoop2;
    (void)dummy;
    (void)KEY_ADR;
    (void)OFS_ADR;
    (void)MAX_CNT2;
 

Offline gerbay

  • Regular Contributor
  • *
  • Posts: 85
  • Country: tr
Re: Weird things in vendor library code...
« Reply #5 on: August 08, 2025, 10:54:33 pm »
RA4M  arm core uses "little-endian" byte order. but some renesas peripheral register uses big-endian byte order. so it needs swap variable.

Code: [Select]
uint32_t change_endian_long (uint32_t a)
{
    return __REV(a);
}

Quote
REV
You can use this instruction to change endianness. REV converts 32-bit big-endian data into little-endian data or 32-bit little-endian data into big-endian data.
 

Offline westfwTopic starter

  • Super Contributor
  • ***
  • Posts: 4639
  • Country: us
Re: Weird things in vendor library code...
« Reply #6 on: August 08, 2025, 11:38:28 pm »
Quote
There are variables defined but not used.
Obviously.  But why are they even defined?

Quote
[endianness] so it needs swap variable.
That would be fine, IF it:
  • was an inlined function. (since it's single instruction on ARMv7m
  • wasn't swappinga constant (which should happen at compile time)
  • wasn't ZERO, which obviously doesn't need byteswapping.
 

Offline ataradov

  • Super Contributor
  • ***
  • Posts: 12463
  • Country: us
    • Personal site
Re: Weird things in vendor library code...
« Reply #7 on: August 08, 2025, 11:40:41 pm »
The zero value may be also auto-generated, so they have byte swap in the template.

Auto-generation of crap code is the base of automotive software, so no surprise here.
Alex
 

Online SiliconWizard

  • Super Contributor
  • ***
  • Posts: 17773
  • Country: fr
Re: Weird things in vendor library code...
« Reply #8 on: August 09, 2025, 12:28:59 am »
Yes it all looks like auto-generated code.
 

Online thm_w

  • Super Contributor
  • ***
  • Posts: 9762
  • Country: ca
  • Non-expert
Re: Weird things in vendor library code...
« Reply #9 on: August 09, 2025, 01:15:50 am »
   
  • wasn't ZERO, which obviously doesn't need byteswapping.

It makes sense to leave it there in case someone comes later and edits the value, IMO.
The function call though I'm not sure, would expect one op code, REV.
Profile -> Modify profile -> Look and Layout ->  Don't show users' signatures
 
The following users thanked this post: Siwastaja

Offline Siwastaja

  • Super Contributor
  • ***
  • Posts: 11140
  • Country: fi
Re: Weird things in vendor library code...
« Reply #10 on: August 09, 2025, 07:18:22 am »
Who makes endianness swap a non-static, non-inlined library function? That's outright crazy. Endianness swap is the textbook example for a macro or inline function.
 

Offline westfwTopic starter

  • Super Contributor
  • ***
  • Posts: 4639
  • Country: us
Re: Weird things in vendor library code...
« Reply #11 on: August 09, 2025, 09:12:13 pm »
could not be bothered to come up with good templates.

I did a bit of additional checking.
`iLoop1` appears only in the crypto code. (600+ occurrences)
It never appears in any context other than the definition and the `(void)iLoop1;` statements.

Maybe it's used for code coverage testing (with different compiler options?_, which I'd expect is especially important when it comes to crypto?
 

Offline Nominal Animal

  • Super Contributor
  • ***
  • Posts: 8349
  • Country: fi
    • My home page and email address
Re: Weird things in vendor library code...
« Reply #12 on: August 10, 2025, 03:09:20 am »
For reversing byte order in variables and compile-time constants:

According to godbolt.org, __builtin_bswap16(), __builtin_bswap32(), and __builtin_bswap64() are supported by GCC, Clang, and ICC.

I would prefer to use those over open-coded versions, because the compiler is more likely to optimize these to optimal machine code.  Simply put, something like
Code: [Select]
#include <stdint.h>

// When neither or both of REV_BUILTINS and REV_OPENCODED are defined, autodetect based on compiler major version.
#if (!defined(REV_BUILTINS) && !defined(REV_OPENCODED)) || (defined(REV_BUILTINS) && defined(REV_OPENCODED))
#undef  REV_BUILTINS
#undef  REV_OPENCODED

// Built-in support since GCC 5.x.x
#if defined(__GNUC__) && __GNUC__ >= 5
#define  REV_BUILTINS
#undef   REV_OPENCODED

// Built-in support since ICC 13.x.x
#elif defined(__INTEL_COMPILER) && __INTEL_COMPILER >= 1300
#define  REV_BUILTINS
#undef   REV_OPENCODED

// Built-in support since clang 10.x.x
#elif defined(__clang__) && __clang_major__ >= 10
#define  REV_BUILTINS
#undef   REV_OPENCODED

// No built-in support
#else
#undef   REV_BUILTINS
#define  REV_OPENCODED
#endif

// Implementation of the rev16(), rev32(), and rev64() functions.
// These reverse the byte order (between little-endian and big-endian).

#if defined(REV_BUILTINS)
// Use compiler-provided __builtin_bswapN() built-in functions

#define  rev16(v)  __builtin_bswap16(v)
#define  rev32(v)  __builtin_bswap32(v)
#define  rev64(v)  __builtin_bswap64(v)

#elif defined(REV_OPENCODED)
// Use open-coded byte order reversing functions

static inline uint_fast16_t  rev16(uint_fast16_t  v) {
    return ((v >> 8) & 0xFF) | ((v & 0xFF) << 8);
}

static inline uint32_t  rev32(uint32_t  v) {
    v = ((v >>  8) & 0x00FF00FF) | ((v & 0x00FF00FF) << 8);
    v = ((v >> 16) & 0x0000FFFF) | ((v & 0x0000FFFF) << 16);
    return v;
}

static inline uint64_t  rev64(uint64_t  v) {
    v = ((v >>  8) & UINT64_C(0x00FF00FF00FF00FF)) | ((v & UINT64_C(0x00FF00FF00FF00FF)) << 8);
    v = ((v >> 16) & UINT64_C(0x0000FFFF0000FFFF)) | ((v & UINT64_C(0x0000FFFF0000FFFF)) << 16);
    v = ((v >> 32) & UINT64_C(0x00000000FFFFFFFF)) | ((v & UINT64_C(0x00000000FFFFFFFF)) << 32);
    return v;
}

#else
#error BUG: Neither REV_BUILTINS nor REV_OPENCODED got defined.
#endif
When neither or both of REV_BUILTINS and REV_OPENCODED are defined, the built-ins are used on GCC 5.x.x and later, ICC 13.x.x and later, and clang 10.x.x and later, and the open-coded ones otherwise.  Otherwise, the defined one determines which are used.  The BUG: error should never occur, but is there in case a later edit causes some arch to be missed.

In all cases, when compilation succeeds, you get rev16(), rev32(), and rev64(), which reverse the byte order in their argument.  For signed arguments, you'll want to cast the return value to the appropriate signed type (intN_t or int_fastN_t).

For code that wants or needs to support random byte orders in e.g. binary files, I do like to implement functions that take a bit pattern of \$k = \lfloor\log_2(N/8)\rfloor\$ bits for revN, each bit enabling or disabling the operation on each line in the open-coded variants.  Then, each byte order (for specific size of value) is represented by a different \$k\$-bit value:  1 bit for 16-bit values; 2 bits for 32-bit values; 3 bits for 64-bit values; 4 bits for 128-bit values; and so on.  I have a prototype value for each type in a file header, and test all possible byte orders in a loop until the prototype value is parsed as the expected logical value, so they also act as file format identifiers.  It does incur a small penalty for multi-byte fields, but with current processors and microcontrollers, that overhead tends to be irrelevant compared to the storage/communications I/O speeds.
« Last Edit: August 10, 2025, 03:13:22 am by Nominal Animal »
 
The following users thanked this post: thm_w, SiliconWizard


Share me

Digg  Facebook  SlashDot  Delicious  Technorati  Twitter  Google  Yahoo
Smf