Remove possible truncation at implicit conversion to type "unsigned char"

[vflouriot]

Inside aes.c line 189 : possible truncation at implicit conversion to type "unsigned char"

// Function RotWord() { k = tempa[0]; tempa[0] = tempa[1]; tempa[1] = tempa[2]; tempa[2] = tempa[3]; tempa[3] = k; }

[kokke]

I don't understand the warning you're quoting - tempa[3] is already an unsigned char to begin with. Sounds spurious... Line 155: uint8_t tempa[4]; // Used for the column/row operations

What tool gave you this warning?

[vflouriot]

Hello Kokke,

The compiler is Tasking Tricore v6.2r2

"k" is defined without type, and as per C language "int" is selected as default, witch is a non portable type (int is 32 bit wide on Aurix).

So the compiler is right to warn because allocating k to tempa[3] may cause a loss of information.

I do think you should have wrote : tempa[3] = (uint8_t)k;

Vincent

[Nable80]

Strictly speaking, k was defined with a type (unsigned [int]). It still makes sense to use another temporary variable here, some variable with a right type (uint8_t) to avoid expansion in line k = tempa[0] that may cost something for 8-bit MCUs.

[kokke]

"k" is defined without type, and as per C language "int" is selected as default, witch is a non portable type (int is 32 bit wide on Aurix).

k is not defined without type. It's defined as unsigned which is implicitly the same as unsigned int.

The code assigning to k basically looks like this:

  unsigned k;
  int i;
  for (i = Nk; i < Nb * (Nr + 1); ++i)
  {
    k = (i - 1) * 4;
  }

Nb is always 4. Nk is 4 for AES256, 6 for AES192 and 8 for AES128. Nr is 10 for AES256, 12 for AES192 and 14 for AES128.

That means k cannot get any larger than 232, which will fit in an uint8_t without problems.

So the compiler is right to warn because allocating k to tempa[3] may cause a loss of information.

I do think you should have wrote : tempa[3] = (uint8_t)k;

It is simply not possible for k to grow large enough that there is any loss of information.

This warning is a false alert.

[vflouriot]

Hi Kokke you are both right and wrong.

The compiler warning message does not says there is a loss of information, it says that this line could cause loss of information depending of a context he's not able to check by itself. (message says "possible truncation...")

He is definitely right to warn. Allocate a int into a byte do may cause a loss of information.

Once you have checked the content of k will always fit into a unint_8 you need to tell the compiler that you did the check and you agree this variable should be explicitely casted from int to unint_8.

Writing (uint8_t)k instead of k will not change any bit into generated machine code but will just prevent the compiler issuing the warning.

Explicit casting makes somewhere a cleaner code because you show that you have checked and you agree to cast. In some other conditions this could cause a real bug.

[DamonHD]

I agree that the cast should be added so that the compiler knows that you know that it is OK to truncate to 8 bits.

(I spend too much of my life porting C/C++ code between platforms, giving clear heads up to other programmers and the compilers that you are clear what is happening is good.)

Rgds

Damon

[Nable80]

The code assigning to k basically looks like this: [...]

This is not the place where compiler complains. Questionable assignments are here: k = tempa[0]; ... tempa[3] = k; Of course, data loss isn't possible here but reusing k as a byte storage in this place is questionable (both due to different size and different meaning of variables).

[kokke]

This commit hopefully solves all grief :)

https://github.com/kokke/tiny-AES-c/commit/691aa01d4ab80c3dcc50f36408f0383f785e1977#diff-9789999dc6dd261657c63f757d1e680d

[vflouriot]

Hi Kokke thanks a lot this definitely remove the warning at my side.

Vincent

[DamonHD]

Paranoid pedant that I am, I would make that new declaration const, since you know that the initial value should not be overridden!

const uint8_t tmpu8 = tempa[0];

Rgds

Damon

[kokke]

Hi @DamonHD - that's a good idea. I've added that in a new commit: https://github.com/kokke/tiny-AES-c/commit/0677e48a4980cc3695bc0f4dab89bad8708d16ea

[kokke]

@vflouriot I never did say thank you for reporting this issue, but thanks for taking an interest in improving the code base :)

[vflouriot]

Thanks You're welcome. Vincent