search

bao-project / bao-hypervisor

Bao, a Lightweight Static Partitioning Hypervisor
Apache License 2.0
379 stars 128 forks source link

fix(armv8): add missing explicit casts #171

Closed joaopeixoto13 closed 2 months ago

josecm commented 2 months ago

@joaopeixoto13 Can you explain further why these casts are required?

joaopeixoto13 commented 2 months ago

@josecm On May 23, this PR introduced additional compiler flags, including -Wconversion, which prompts the compiler to generate warnings for implicit type conversions that could lead to potential issues. This PR includes the necessary explicit casts for the ARMv8 architecture to address these warnings.

josecm commented 2 months ago

@josecm On May 23, this PR introduced additional compiler flags, including -Wconversion, which prompts the compiler to generate warnings for implicit type conversions that could lead to potential issues. This PR includes the necessary explicit casts for the ARMv8 architecture to address these warnings.

I understand but I don't have any issue in compiling the current main in my machine. I am using aarch64-none-elf-gcc (Arm GNU Toolchain 13.2.rel1 (Build arm-13.7)) 13.2.1 20231009. Are you using a more recent compiler?

joaopeixoto13 commented 2 months ago

@josecm On May 23, this PR introduced additional compiler flags, including -Wconversion, which prompts the compiler to generate warnings for implicit type conversions that could lead to potential issues. This PR includes the necessary explicit casts for the ARMv8 architecture to address these warnings.

I understand but I don't have any issue in compiling the current main in my machine. I am using aarch64-none-elf-gcc (Arm GNU Toolchain 13.2.rel1 (Build arm-13.7)) 13.2.1 20231009. Are you using a more recent compiler?

I am using the aarch64-none-elf-gcc (GNU Toolchain for the Arm Architecture 11.2-2022.02 (arm-11.14)) 11.2.1 20220111, which is the same version referenced in Appendix III of the bao-demos guide.

josecm commented 2 months ago

I am using the aarch64-none-elf-gcc (GNU Toolchain for the Arm Architecture 11.2-2022.02 (arm-11.14)) 11.2.1 20220111, which is the same version referenced in Appendix III of the bao-demos guide.

This is a minimal version and in need of an update. The compiler to be used should be the one in bao-ci's container. If you compile with this version we shouldn't run into any issues with those flags.

joaopeixoto13 commented 2 months ago

I am using the aarch64-none-elf-gcc (GNU Toolchain for the Arm Architecture 11.2-2022.02 (arm-11.14)) 11.2.1 20220111, which is the same version referenced in Appendix III of the bao-demos guide.

This is a minimal version and in need of an update. The compiler to be used should be the one in bao-ci's container. If you compile with this version we shouldn't run into any issues with those flags.

Understood, I will update the compiler accordingly. However, would it not be more prudent to include these explicit conversions in the code? Or, given that the compiler handles these automatically, do you believe it is unnecessary?

josecm commented 2 months ago

Understood, I will update the compiler accordingly. However, would it not be more prudent to include these explicit conversions in the code? Or, given that the compiler handles these automatically, do you believe it is unnecessary?

I think @miguelafsilva5 should answer that.

miguelafsilva5 commented 2 months ago

I believe that if the most recent compiler does not issue any error you should not include those casts. Otherwise we would also have to include other explictit casts throughout the bao code.

Eventually, if those casts are necessary the MISRA analyzer will spot them.

DavidMCerdeira commented 2 months ago

I'll take @miguelafsilva5 suggestion and close this PR