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

[BUG] Linux Chip-tool GCC and Chip-tool clang do not link the same Logging implementation #30633

Closed
jmartinez-silabs opened this issue Nov 23, 2023 · 3 comments · Fixed by #32119
Labels
bug Something isn't working linux needs triage

Comments

@jmartinez-silabs
Copy link
Member

jmartinez-silabs commented Nov 23, 2023

Reproduction steps

By building chip-tool with clang on my raspi I noticed the logs had no timestamps
./scripts/build/build_examples.py --target linux-arm64-chip-tool-ipv6only-clang build

CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_tool_kvs
CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_kvs
CHIP:DL: ChipLinuxStorage::Init: Attempt to re-initialize with KVS config file: /tmp/chip_kvs

with GCC, timestamps are present.
./scripts/build/build_examples.py --target linux-arm64-chip-tool-ipv6only build

[1700760123.127980][202484:202484] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_tool_kvs
[1700760123.129199][202484:202484] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_kvs
[1700760123.129238][202484:202484] CHIP:DL: ChipLinuxStorage::Init: Attempt to re-initialize with KVS config file: /tmp/chip_kvs

When built with clang, src/platform/logging/impl/stdio/Logging.cpp is used/linked.
when GCC is used, src/platform/Linux/Logging.cpp is used/linked.

While src/platform/Linux/Logging.cpp should always be used in this case, this is not the only problem.
Both LogV functions seem compiled, the same symbol is defined twice and the linker doesn't throw an error.

A quick test @andy31415 and I did:

  1. Current situation, Clang and GCC link a different LogV implementation.
    src/platform/logging/impl/stdio/Logging.cpp (CLANG), src/platform/Linux/Logging.cpp (GCC)
  2. Comment out or rename one implementation; Clang and GCC link the same remaining LogV.
  3. Comment out or rename both LogV functions; Clang and GCC complain of the missing symbol.

mapfiles.zip

The issue might be linked with #23528

Bug prevalence

Always

GitHub hash of the SDK that was being used

6b91619

Platform

raspi, other

Platform Version(s)

No response

Anything else?

No response

@jmartinez-silabs jmartinez-silabs added bug Something isn't working needs triage labels Nov 23, 2023
@github-actions github-actions bot added the linux label Nov 23, 2023
@andy31415
Copy link
Contributor

I think we are not allowed to try to add extra dependencies top level to "override". Instead if seems pigweed defines global backend targets that are then used. We should probably do the same for logging.

@ksperling-apple
Copy link
Contributor

IIRC I added the force_stdio target because of how the gn dependencies for certain targets are set up. I think certain library targets end up pulling in the platform default backend (which on Darwin is a no-op, since the integration into os_log happens in the logging "front end" macros), but then certain tools or tests that depend on that library target actually want to have logging to stdout at runtime.

An alternative would be for those tools to install a logging redirection callback to copy messages to stderr instead of replacing the backend. IIRC I looked at that but didn't do it at the time because it would then cause double logging on Linux where the default backend already logs to stdio. Maybe we could make the redirection approach work if we expose some sort of "LogVIsStdio" flag so we can not install the redirection callback in that case.

@ksperling-apple
Copy link
Contributor

ksperling-apple commented Nov 24, 2023

Actually I'm a bit hazy on exactly what the stumbling block for using the redirection approach was... It might have been unit tests where there wasn't necessarily a clean place to set up the redirection. I think for platforms like Linux or Darwin the question is really where the best place is to decide whether or not logging should go to stdio (in addition to a system logging backend), it doesn't seem correct for a library to just log to stderr -- though of course we're not actually shipping any pre-built static or dynamic libraries directly out of the SDK source at this point.

Tangentially related, for chip-tool specifically it would be useful if we actually had some concept of command line output distinct from logging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working linux needs triage
Projects
None yet
3 participants