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

Incorrect use of __attribute__((weak)) in flash_hal #8883

Closed
6 tasks done
ErikBogg opened this issue Mar 8, 2023 · 7 comments
Closed
6 tasks done

Incorrect use of __attribute__((weak)) in flash_hal #8883

ErikBogg opened this issue Mar 8, 2023 · 7 comments

Comments

@ErikBogg
Copy link
Contributor

ErikBogg commented Mar 8, 2023

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: other
  • Core Version: 3.1.1
  • Development Env: Arduino IDE
  • Operating System: Windows

Settings in IDE

  • Module: Generic ESP8266 Module

Problem Description

When adding the line:
FLASH_MAP_SETUP_CONFIG(FLASH_MAP_MAX_FS)
in my sketch, I get a "multiple definition of `__flashdesc';"

The problem is that the assignment of __flashdesc on line 46 in flash_hal.h does not contain the 'attr' argument passed to the macro. So when the line 75 in flash_hal.cpp:
FLASH_MAP_SETUP_CONFIG_ATTR(__attribute__((weak)), FLASH_MAP_OTA_FS)
assigns __flashdesc to FLASH_MAP_OTA_FS, there is no weak assignment which makes it impossible override in user application.

Work-around until it is solved

Remove line 75 in flash_hal.cpp

Links to similar topics

https://forum.arduino.cc/t/multiple-definition-of-flashdesc-when-using-flash-map-setup-config/1098083/1

@ErikBogg
Copy link
Contributor Author

ErikBogg commented Mar 8, 2023

Patch to solve the issue:

diff --git a/cores/esp8266/flash_hal.h b/cores/esp8266/flash_hal.h
index 682e1dcf..061b1d0b 100644
--- a/cores/esp8266/flash_hal.h
+++ b/cores/esp8266/flash_hal.h
@@ -43,7 +43,7 @@ extern const flash_map_s __flashdesc[];

 #define FLASH_MAP_SETUP_CONFIG(conf) FLASH_MAP_SETUP_CONFIG_ATTR(,conf)
 #define FLASH_MAP_SETUP_CONFIG_ATTR(attr, conf...) \
-  const flash_map_s __flashdesc[] PROGMEM = conf; \
+  extern const flash_map_s __flashdesc[] PROGMEM attr = conf; \
   const char *flashinit (void) attr; \
   const char *flashinit (void) \
   { \

@ErikBogg
Copy link
Contributor Author

ErikBogg commented Mar 8, 2023

#8884

@d-a-v
Copy link
Collaborator

d-a-v commented Mar 10, 2023

I wonder why extern is needed on your setup, one should get this warning:
warning: ‘__flashdesc’ initialized and declared ‘extern’.
What happens when you omit extern ?

But I agree on the missing attr, and I don't explain yet why it is not needed on my setup.

@ErikBogg
Copy link
Contributor Author

Without extern you have an error like:
flash_hal.h:46:21: error: weak declaration of '__flashdesc' must be public

@mcspr
Copy link
Collaborator

mcspr commented Mar 11, 2023

why it is not needed on my setup.

Multiple compilation units using flash_hal.h? Not using either SPIFFS of LittleFS won't trigger the issue.

Without extern you have an error like:

I think the question is why we have to write extern when it is already a public symbol.
And the answer is - GCC has a bug :)
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83271

@d-a-v
Copy link
Collaborator

d-a-v commented Mar 11, 2023

Agreed, I can indeed reproduce when effectively using a FS.

@d-a-v
Copy link
Collaborator

d-a-v commented Mar 11, 2023

Closing per #8884, thanks @ErikBogg

@d-a-v d-a-v closed this as completed Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants