-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Move out the weak functions of MatterGetAccessPrivilegeFor_XX #21159
Move out the weak functions of MatterGetAccessPrivilegeFor_XX #21159
Conversation
PR #21159: Size comparison from 2557f51 to 91fe555 Increases (2 builds for cc13x2_26x2, esp32)
Decreases (4 builds for bl602, cc13x2_26x2, nrfconnect)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
src/app/AccessPrivilegeGet.cpp
Outdated
@@ -0,0 +1,38 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a better filename, which would indicate that these are here just to be overridden. DefaultRestrictivePrivilegeGetters
or something like that, perhaps?
But I have to ask: why do we need these at all? Why aren't all consumers just providing these functions, so we can avoid weak symbols altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the filename.
As described in #16540, RequiredPrivilege is built into the DM/IM library, but the underlying data is built per app/example. If we remove the weak functions there will be compilation errors for the chip-im-responder/initiator. But if we place the weak functions in RequiredPrivilege.cpp
, the functions in src/app/utils/privilege-storage.cpp
might not be able to override the weak functions for the apps/examples.
We only called the MatterGetAccessPrivilegeForXX
functions in RequiredPrivilege.h
which is included by RequiredPrivilege.cpp
, so the linker might ignore the override functions and use the weak functions intead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of that answers my question: why aren't all consumers providing these functions? That was the initial intent; unfortunately @mlepage-google has not been able to get to it. But we should implement that initial intent instead of adding more workarounds for the fundamental weak-function brokenness....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created another PR #21220 removing the weak functions and adding the stubs for test:helper
& chip-im-responder/initiator
.
Close since #21220 merged |
Problem
#19706
The
MatterGetAccessPrivilegeFor_XX
in thesrc/app/util/privilege-storage.cpp
sometimes does not override the weak functions insrc/app/RequiredPrivilege.cpp
. Looks like we call theGetAccessPrivilege
functions only in the included header filesrc/app/RequiredPrivilege.h
. As described in #20538 , the linker might just link the first found (weak) function codes and will not link the strong functions in another file objects anymore.Change overview
Move the weak functions out of the
src/app/RequiredPrivilege.cpp
.Testing
CI