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

sprintf statement only gives compile error if compiled with compiler warning level "All" and "More" (-Wall -Werror=all) #7024

Closed
ArminJo opened this issue Jul 21, 2022 · 42 comments · Fixed by #9273
Assignees
Labels
Status: To be implemented Selected for Development Type: Bug 🐛 All bugs
Milestone

Comments

@ArminJo
Copy link

ArminJo commented Jul 21, 2022

Board

compiler problem

Device Description

compiler problem

Hardware Configuration

compiler problem

Version

v2.0.4

IDE Name

Arduino, Sloeber

Operating System

%

Flash frequency

compiler problem

PSRAM enabled

no

Upload speed

compiler problem

Description

The sketch below throws the error

E:\WORKSPACE_ARDUINO\ArduinoSketches\sketch_jul21a\sketch_jul21a.ino: In function 'void setup()':
sketch_jul21a:7:26: error: '%s' directive writing up to 19 bytes into a region of size 14 [-Werror=format-overflow=]
   sprintf(sStringBuffer, "Acc.X %s", &sStringBuffer[10]);
   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E:\WORKSPACE_ARDUINO\ArduinoSketches\sketch_jul21a\sketch_jul21a.ino:7:10: note: 'sprintf' output between 7 and 26 bytes into a destination of size 20
   sprintf(sStringBuffer, "Acc.X %s", &sStringBuffer[10]);
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus.exe: some warnings being treated as errors
exit status 1
'%s' directive writing up to 19 bytes into a region of size 14 [-Werror=format-overflow=]

if compiled with -Wall -Werror=all

For all non ESP32 platforms it compiles without any error! - see https://github.com/ArminJo/Arduino-BlueDisplay/actions/runs/2714739545

Sketch

char sStringBuffer[20];

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  sprintf(&sStringBuffer[10], "123.4567"); // edited: original code was: dtostrf(123.45, 4, 1, &sStringBuffer[10]);
  sprintf(sStringBuffer, "Acc.X %s", &sStringBuffer[10]);
  Serial.println(sStringBuffer);
}

void loop() {
  // put your main code here, to run repeatedly:
}

Debug Message

E:\WORKSPACE_ARDUINO\ArduinoSketches\sketch_jul21a\sketch_jul21a.ino: In function 'void setup()':
sketch_jul21a:7:26: error: '%s' directive writing up to 19 bytes into a region of size 14 [-Werror=format-overflow=]
   sprintf(sStringBuffer, "Acc.X %s", &sStringBuffer[10]);
   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E:\WORKSPACE_ARDUINO\ArduinoSketches\sketch_jul21a\sketch_jul21a.ino:7:10: note: 'sprintf' output between 7 and 26 bytes into a destination of size 20
   sprintf(sStringBuffer, "Acc.X %s", &sStringBuffer[10]);
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus.exe: some warnings being treated as errors
exit status 1
'%s' directive writing up to 19 bytes into a region of size 14 [-Werror=format-overflow=]


### Other Steps to Reproduce

_No response_

### I have checked existing issues, online documentation and the Troubleshooting Guide

- [X] I confirm I have checked existing issues, online documentation and Troubleshooting guide.
@ArminJo ArminJo added the Status: Awaiting triage Issue is waiting for triage label Jul 21, 2022
@SuGlider SuGlider self-assigned this Jul 22, 2022
@SuGlider
Copy link
Collaborator

@ArminJo - I can't rerproduce the issue.

I was able to successfully compile the proposed sketch and upload the firmware to an ESP32-DevKit board.
The output is:

ets Jun  8 2016 00:22:57

rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
flash read err, 1000
ets_main.c 371 
ets Jun  8 2016 00:22:57

rst:0x10 (RTCWDT_RTC_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:1
load:0x3fff0030,len:1344
load:0x40078000,len:13516
load:0x40080400,len:3604
entry 0x400805f0
Acc.X 123.5

@SuGlider SuGlider added Resolution: Unable to reproduce With given information issue is unable to reproduce and removed Status: Awaiting triage Issue is waiting for triage labels Jul 24, 2022
@SuGlider
Copy link
Collaborator

My suggestion is to remove the ESP32 board from the IDE and reinstall it again.

@ArminJo
Copy link
Author

ArminJo commented Jul 24, 2022

Du you set compiler warning level to error???
And in the log in the link, the ESP board is installed freshly ad hoc!

@SuGlider
Copy link
Collaborator

There is no way to set Compiler warning to error in the Arduino IDE.
Anyway, there is no warning message from the compiler when compiling this sketch.

@ArminJo
Copy link
Author

ArminJo commented Jul 24, 2022

Sorry I meant ALL like in the topic
2022-07-25 00_04_22-Preferences
.

@ArminJo
Copy link
Author

ArminJo commented Jul 24, 2022

Use File > Preferences

@SuGlider
Copy link
Collaborator

Ok, thanks, indeed, with this setup I see the error message.

@ArminJo
Copy link
Author

ArminJo commented Jul 24, 2022

Great 👍

@SuGlider
Copy link
Collaborator

I guess you are reading some IMU data for its Axis.
This issue can be solved by an equivalent code using sprintf().
It compiles with the preferences suggested setup and runs fine.

  //dtostrf(123.45, 4, 1, &sStringBuffer[10]);
  //sprintf(sStringBuffer, "Acc.X %s", &sStringBuffer[10]);
  float ff = 123.46;    // 123.45 results in "123.4" and 123.46 results in "123.5"
  sprintf(sStringBuffer, "Acc.X %4.1f", ff);
  Serial.println(sStringBuffer);

@SuGlider
Copy link
Collaborator

SuGlider commented Jul 24, 2022

For testing you can directly use:
Serial.printf("Acc.X %4.1f", 123.45);

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  Serial.printf("Acc.X %4.1f", 123.45);
}

void loop() {
  // put your main code here, to run repeatedly:
}

@ArminJo
Copy link
Author

ArminJo commented Jul 24, 2022

Thanks,
I know how to program and how to avoid this compiler bug.
But all compilers for all other platforms know how to compile this program without errors except the ESP32 compiler.

@ArminJo ArminJo closed this as completed Jul 24, 2022
@SuGlider SuGlider added Status: Needs investigation We need to do some research before taking next steps on this issue and removed Resolution: Unable to reproduce With given information issue is unable to reproduce labels Jul 24, 2022
@SuGlider
Copy link
Collaborator

@VojtechBartoska - Could you please address this issue with the Espressif Compiler team - if there is any - not sure.

@igrr
Copy link
Member

igrr commented Jul 25, 2022

Hi folks, as far as I can tell, sufficiently new versions of GCC produce the same error for other architectures.
You can see the result here: https://godbolt.org/z/6e9MP4Mff. The panel on the right shows that the code fails to build for a few other architectures, with GCC version 8 and above. It compiles successfully with GCC 5.

The difference between the Arduino cores might be in:

  • compiler version used
  • warning flags enabled by each core

Indeed, esp32-arduino core adds -Werror=all to -Wall flag used by other cores.
Compare:

So for other cores, only a warning is reported when a sufficiently new version of the compiler is used, while for arduino-esp32 the warning is also treated as an error.

The reason why this didn't occur previously is that compiler.warning_flags options were not added to the final compiler flags. This was done in #6596.

-Werror was added to the flags in commit f6d4843, however I haven't found the explanation why this was done.

@igrr igrr added Type: Bug 🐛 All bugs Status: To be implemented Selected for Development and removed Status: Needs investigation We need to do some research before taking next steps on this issue labels Jul 25, 2022
@ArminJo
Copy link
Author

ArminJo commented Jul 25, 2022

Hi Ivan,
then at least ESP should add -Wno-error=format-overflow as I have done here to avoid this nasty bug,

@igrr igrr reopened this Jul 25, 2022
@igrr
Copy link
Member

igrr commented Jul 25, 2022

I'm not sure of the exact way to resolve the issue, it's up to the arduino-esp32 team to decide.

Putting my C developer hat on, I'd say that erroring out on format warnings (as most other -Wall warnings) is in general a good idea. Compilers do often report issues which, if unnoticed, cause the program to work incorrectly. That said, there are two things against this:

  1. given that there are plenty of other arduino cores out there, arduino-esp32 should try not to deviate much from the norm, if only to make it easier for library developers to write code compatible with multiple cores
  2. in this specific case, the warning is a false positive; however the compiler doesn't know about the semantics of the non-standard dtostrf function to understand this.

@ArminJo
Copy link
Author

ArminJo commented Jul 25, 2022

If you replace the unknown
dtostrf(123.45, 4, 1, &sStringBuffer[10]);
by the known
sprintf(&sStringBuffer[10], "123.4567");
you get the same error 😭

@VojtechBartoska VojtechBartoska added this to the 2.1.0 milestone Jul 25, 2022
@ArminJo ArminJo reopened this Oct 12, 2022
@mrengineer7777
Copy link
Collaborator

It looks like #7060 hasn't been merged yet. They are targeting it for 2.0.6 release.

@VojtechBartoska
Copy link
Collaborator

It looks like #7060 hasn't been merged yet. They are targeting it for 2.0.6 release.

Yes, still in progress and will be released in 2.0.6

@ArminJo
Copy link
Author

ArminJo commented Oct 12, 2022

Thanks for the info 👍

@VojtechBartoska
Copy link
Collaborator

Seems this issue can be closed as solved, if needed you can reopen.

@ArminJo
Copy link
Author

ArminJo commented Feb 20, 2024

If I compile 2.0.14 without -Wno-error=format-overflow I still get the errors 😢 !
See https://github.com/ArminJo/Arduino-BlueDisplay/actions/runs/7975089958/job/21772457525#step:3:420
And I cannot reopen the issue, only comment it.

@igrr igrr reopened this Feb 20, 2024
@igrr
Copy link
Member

igrr commented Feb 20, 2024

Reopened as the linked PR hasn't been merged.

@VojtechBartoska VojtechBartoska removed this from the 2.0.5 milestone Feb 20, 2024
@VojtechBartoska
Copy link
Collaborator

I see, keeping it in Backlog.

@me-no-dev
Copy link
Member

@ArminJo can you try with 3.0.0-alpha2 ? And make sure you do not enable all errors in the ArduinoIDE menu (arduino-cli)

@me-no-dev
Copy link
Member

@igrr -Werror=all is enabled in 2.0.14 and 3.0.0 only if all warnings are enabled. In all other cases this flag is not enabled

@me-no-dev
Copy link
Member

@ArminJo https://github.com/ArminJo/Arduino-BlueDisplay/actions/runs/7975089958/job/21772457525#step:3:303 you have enabled all warnings, therefore I see no issue here?

@ArminJo
Copy link
Author

ArminJo commented Feb 21, 2024

@me-no-dev Strange definition of an issue....
And keep in mind that all other cores do not have this problem.

@me-no-dev
Copy link
Member

me-no-dev commented Feb 21, 2024

@ArminJo as far as I see, the issue comes from lines like sprintf_P(sStringBuffer, PSTR("Yaw %s"), &sStringBuffer[10]); and the error reads error: '%s' directive writing up to 19 bytes into a region of size 14 [-Werror=format-overflow=]. Now I see where the compiler gets confused, but how is that in any way fault of the flags or the core?

You can either use another buffer to store the dtostrf value or file an issue report in the toolchain repository to see why counts the source size as 19, instead of 9.

Does that make sense?

@igrr
Copy link
Member

igrr commented Feb 21, 2024

or file an issue report in the toolchain repository to see why counts the source size as 19, instead of 9.

I don't think this is necessary, as the behavior is exactly the same for GCC on other platforms. Here's an example with a really old ARM GCC 8.3.1: https://godbolt.org/z/6bcEvMPnf

@ArminJo
Copy link
Author

ArminJo commented Feb 21, 2024

@me-no-dev It makes no sense to change a correct code which compiles everywhere without errors, except on ESP32.
ESP32 is not the center of the world.
This is my last comment to it!

@igrr
Copy link
Member

igrr commented Feb 21, 2024

I think we need to compare the compiler arguments used by other Arduino cores with the ones we emit. Based on the godbolt link above, it doesn't look like a compiler-related difference. Probably we have a different subset of warnings enabled?

I do agree that the inconsistent warnings/errors make the work of library maintainers harder, we should try to produce the behavior similar to other Arduino cores if possible.

@igrr
Copy link
Member

igrr commented Feb 21, 2024

@me-no-dev looking at 2.0.14, we do enable -Werror=all for "More" and "All" warning options

arduino-esp32/platform.txt

Lines 125 to 126 in 44da992

compiler.warning_flags.more=-Wall -Werror=all
compiler.warning_flags.all=-Wall -Werror=all -Wextra

Whereas other cores don't do this:

The point of #7060 which was closed without merging was to only add -Werror=all in CI builds, and keep the user-facing warnings behavior same as for the other cores.

@me-no-dev
Copy link
Member

Yup. I have approached this problem from a totally wrong direction up until now. On it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment