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

IPAddress::operator= needs 32-bit aligned pointer #8817

Closed
4 of 6 tasks
TD-er opened this issue Jan 18, 2023 · 7 comments · Fixed by #8818
Closed
4 of 6 tasks

IPAddress::operator= needs 32-bit aligned pointer #8817

TD-er opened this issue Jan 18, 2023 · 7 comments · Fixed by #8818

Comments

@TD-er
Copy link
Contributor

TD-er commented Jan 18, 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: [ESP-12|ESP-01|ESP-07|ESP8285 device|other]
  • Core Version: [Current master / 2.7.4]
  • Development Env: [Platformio]
  • Operating System: [Windows|Ubuntu]

Settings in IDE

  • Module: [Generic ESP8266 Module]
  • Flash Mode: [qio|dio|other]
  • Flash Size: [4MB/1MB]
  • lwip Variant: [v1.4|v2 Lower Memory|Higher Bandwidth]
  • Reset Method: [ck|nodemcu]
  • Flash Frequency: [40Mhz]
  • CPU Frequency: [80Mhz|160MHz]
  • Upload Using: [OTA|SERIAL]
  • Upload Speed: [115200|other] (serial upload only)

Problem Description

LoadStoreAlignmentCause: Load or store to an unaligned address
  epc1=0x402a1358 in IPAddress::operator=(unsigned char const*) at ??:?
en deze stacktrace:
0x4029280d in String::operator=(char const*) at ??:?
0x401012bb in umm_free_core at umm_malloc.cpp:?
0x401015a2 in malloc at ??:?
0x402d1866 in wifi_get_opmode at ??:?
0x402a2568 in spiffs_cache_page_get_by_fd at ??:?
0x402a2568 in spiffs_cache_page_get_by_fd at ??:?
0x402a2568 in spiffs_cache_page_get_by_fd at ??:?
0x402a2568 in spiffs_cache_page_get_by_fd at ??:?
0x4024821f in setConnectionSpeed() at ??:?
0x40248dc2 in prepareWiFi() at ??:?
0x40241468 in WiFiEventData_t::markWiFiBegin() at ??:?
0x40248f29 in AttemptWiFiConnect() at ??:?
0x40205e24 in ESP8266WiFiSTAClass::status() at ??:?
0x40248084 in wifiAPmodeActivelyUsed() at ??:?
0x4024909c in WiFiConnectRelaxed() at ??:?
0x40245ce9 in NetworkConnectRelaxed() at ??:?
0x4024bf23 in ESPEasy_setup() at ??:?
0x40215084 in setup at ??:?
0x40293ddb in loop_wrapper() at core_esp8266_main.cpp:?
0x40100469 in cont_wrapper at ??:?

See the assignment operator and other functions where the const uint8_t* is cast to an const uint32_t*

IPAddress& IPAddress::operator=(const uint8_t *address) {
setV4();
v4() = *reinterpret_cast<const uint32_t*>(address);
return *this;
}

bool IPAddress::operator==(const uint8_t* addr) const {
return isV4() && v4() == *reinterpret_cast<const uint32_t*>(addr);
}

The call is simply like this:

  const IPAddress ip     = Settings.IP;
  const IPAddress gw     = Settings.Gateway;
  const IPAddress subnet = Settings.Subnet;
  const IPAddress dns    = Settings.DNS;

Where all these are part of a larger struct, which begins like this:

  unsigned long PID = 0;
  int           Version = 0;
  int16_t       Build = 0;
  uint8_t       IP[4] = {0};
  uint8_t       Gateway[4] = {0};
  uint8_t       Subnet[4] = {0};
  uint8_t       DNS[4] = {0};

As can be seen, these are indeed not 32-bit aligned.

I think the build toolchain may recently have been updated, as this struct and the code that's now failing has not been changed for at least 4 years.

The same code in IPAddress has not been changed for a while as the current master branch has the same code for these functions as the 2.7.4 we're using.

@mcspr
Copy link
Collaborator

mcspr commented Jan 18, 2023

s/Needs aligned/We need memcpy into u32 and then copy instead of reinterpret cast/?
If you compare with 2.7.4, things definitely have changed b/c of GCC 10.3

@TD-er
Copy link
Contributor Author

TD-er commented Jan 18, 2023

There is already a proper constructor which does copy the bytes one-by-one, so why should there be a reinterpret_cast or memcpy?

By the way, I did a few more tests to let the compiler find all occurences of this operator= and found a few more.
I did make the assignment operator private.

But strangely enough the exact lines as above with const IPAddress ip = Settings.IP; etc. were not found.
This one was: WiFiEventData.dns0_cache = dns; which was one line below in my code. (dns0_cache is of type IPAddress)

So maybe the compiler already may strip this code in the optimizer step?

This is the complete function:
https://github.com/letscontrolit/ESPEasy/blob/0c0175ebed4938aecc21d60ea595af3f88fe9544/src/src/ESPEasyCore/ESPEasyWifi.cpp#L1448-L1468

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 18, 2023

v4() = *reinterpret_cast<const uint32_t*>(address);

Can you try

memcpy_P(&v4(), address, 4);

@TD-er
Copy link
Contributor Author

TD-er commented Jan 18, 2023

OK.. also tried with IPAddress& operator=(uint32_t address); set to private to see what this may show up and then noticed some very strange thing in int ESP8266WiFiGenericClass::hostByName(const char* aHostname, IPAddress& aResult, uint32_t timeout_ms)

aResult = static_cast<uint32_t>(INADDR_NONE); where INADDR_NONE is already of type IPAddress, so casting to uint32_t is not needed.
And what makes it even more strange is that INADDR_NONE is 255.255.255.255 on ESP8266, but 0.0.0.0 on ESP32.

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 18, 2023

And what makes it even more strange is that INADDR_NONE is 255.255.255.255 on ESP8266, but 0.0.0.0 on ESP32.

isSet() is preferred with ESP8266 (which tests both)

@mcspr
Copy link
Collaborator

mcspr commented Jan 18, 2023

Let's just figure out assignments & ctors first. I'll prepare a PR.

@TD-er
Copy link
Contributor Author

TD-er commented Jan 18, 2023

v4() = *reinterpret_cast<const uint32_t*>(address);

Can you try

memcpy_P(&v4(), address, 4);

This is a fix.
Just tested it and with this code it works just fine. reverted it and crashes very reproducible.

Just strange that the lines with the single used const IPAddress ip = Settings.IP; didn't fail to compile with the assignment operator set to private.

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

Successfully merging a pull request may close this issue.

3 participants