[PATCH] linker_lists.h: Add attribute used to ll_entry_start macro
From 39d292a327b104dcb1347afb545b2baeb7b2227e Mon Sep 17 00:00:00 2001
From: AdityaK appujee@google.com Date: Tue, 21 Feb 2023 15:05:54 -0800 Subject: [PATCH] linker_lists.h: Add attribute used to ll_entry_start macro
The variable gets dropped by clang compiler in an optimized builds. Adding attribute((used)) allows the symbol to be preserved. Similar changes have been proposed in the past e.g., 569524741a01e1a96fc2b75dd7e5d12e41ce6c2b for ll_entry_declare macro.
Signed-off-by: AdityaK appujee@google.com --- include/linker_lists.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linker_lists.h b/include/linker_lists.h index d3da9d44e8..4cd13c3bc8 100644 --- a/include/linker_lists.h +++ b/include/linker_lists.h @@ -125,7 +125,7 @@ #define ll_entry_start(_type, _list) \ ({ \ static char start[0] __aligned(CONFIG_LINKER_LIST_ALIGN) \ - __attribute__((unused)) \ + __attribute__((unused)) __attribute__((used)) \ __section("__u_boot_list_2_"#_list"_1"); \ (_type *)&start; \ })
On Tue, 21 Feb 2023 at 16:33, Aditya Kumar appujee@google.com wrote:
From 39d292a327b104dcb1347afb545b2baeb7b2227e Mon Sep 17 00:00:00 2001 From: AdityaK appujee@google.com Date: Tue, 21 Feb 2023 15:05:54 -0800 Subject: [PATCH] linker_lists.h: Add attribute used to ll_entry_start macro
The variable gets dropped by clang compiler in an optimized builds. Adding attribute((used)) allows the symbol to be preserved. Similar changes have been proposed in the past e.g., 569524741a01e1a96fc2b75dd7e5d12e41ce6c2b for ll_entry_declare macro.
Signed-off-by: AdityaK appujee@google.com
include/linker_lists.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Tue, Feb 21, 2023 at 03:33:20PM -0800, Aditya Kumar wrote:
The variable gets dropped by clang compiler in an optimized builds. Adding attribute((used)) allows the symbol to be preserved. Similar changes have been proposed in the past e.g., 569524741a01e1a96fc2b75dd7e5d12e41ce6c2b for ll_entry_declare macro.
Signed-off-by: AdityaK appujee@google.com Reviewed-by: Simon Glass sjg@chromium.org
include/linker_lists.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linker_lists.h b/include/linker_lists.h index d3da9d44e8..4cd13c3bc8 100644 --- a/include/linker_lists.h +++ b/include/linker_lists.h @@ -125,7 +125,7 @@ #define ll_entry_start(_type, _list) \ ({ \ static char start[0] __aligned(CONFIG_LINKER_LIST_ALIGN) \
- __attribute__((unused)) \
- __attribute__((unused)) __attribute__((used)) \ __section("__u_boot_list_2_"#_list"_1"); \ (_type *)&start; \
})
So, saying "unused" and then "used" doesn't seem to make any sense. And given some other problems we see with newer clang, which Simon reports this patch doesn't fully fix, we probably need to give that area a good going over to see what attributes do and don't make sense, really.
So, saying "unused" and then "used" doesn't seem to make any sense.
unused and used attributes do not cancel each other. They have different semantics. I agree this part of the code needs some attention. zero sized arrays are not C compliant as I understand it, even more so when it is declared outside of a struct.
On Wed, Mar 22, 2023 at 4:58 PM Tom Rini trini@konsulko.com wrote:
On Tue, Feb 21, 2023 at 03:33:20PM -0800, Aditya Kumar wrote:
The variable gets dropped by clang compiler in an optimized builds. Adding attribute((used)) allows the symbol to be preserved. Similar changes have been proposed in the past e.g., 569524741a01e1a96fc2b75dd7e5d12e41ce6c2b for ll_entry_declare macro.
Signed-off-by: AdityaK appujee@google.com Reviewed-by: Simon Glass sjg@chromium.org
include/linker_lists.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linker_lists.h b/include/linker_lists.h index d3da9d44e8..4cd13c3bc8 100644 --- a/include/linker_lists.h +++ b/include/linker_lists.h @@ -125,7 +125,7 @@ #define ll_entry_start(_type, _list) \ ({ \ static char start[0] __aligned(CONFIG_LINKER_LIST_ALIGN) \
- __attribute__((unused)) \
- __attribute__((unused)) __attribute__((used)) \ __section("__u_boot_list_2_"#_list"_1"); \ (_type *)&start; \
})
So, saying "unused" and then "used" doesn't seem to make any sense. And given some other problems we see with newer clang, which Simon reports this patch doesn't fully fix, we probably need to give that area a good going over to see what attributes do and don't make sense, really.
-- Tom
On Thu, Mar 23, 2023 at 01:29:29PM -0700, appujee wrote:
So, saying "unused" and then "used" doesn't seem to make any sense.
unused and used attributes do not cancel each other. They have different semantics. I agree this part of the code needs some attention. zero sized arrays are not C compliant as I understand it, even more so when it is declared outside of a struct.
Ah, yeah, at least a comment explaining what and why we're doing what we're doing here, and probably some other parts of the linker list code that need addressing then, please.
On Wed, Mar 22, 2023 at 4:58 PM Tom Rini trini@konsulko.com wrote:
On Tue, Feb 21, 2023 at 03:33:20PM -0800, Aditya Kumar wrote:
The variable gets dropped by clang compiler in an optimized builds. Adding attribute((used)) allows the symbol to be preserved. Similar changes have been proposed in the past e.g., 569524741a01e1a96fc2b75dd7e5d12e41ce6c2b for ll_entry_declare macro.
Signed-off-by: AdityaK appujee@google.com Reviewed-by: Simon Glass sjg@chromium.org
include/linker_lists.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linker_lists.h b/include/linker_lists.h index d3da9d44e8..4cd13c3bc8 100644 --- a/include/linker_lists.h +++ b/include/linker_lists.h @@ -125,7 +125,7 @@ #define ll_entry_start(_type, _list) \ ({ \ static char start[0] __aligned(CONFIG_LINKER_LIST_ALIGN) \
- __attribute__((unused)) \
- __attribute__((unused)) __attribute__((used)) \ __section("__u_boot_list_2_"#_list"_1"); \ (_type *)&start; \
})
So, saying "unused" and then "used" doesn't seem to make any sense. And given some other problems we see with newer clang, which Simon reports this patch doesn't fully fix, we probably need to give that area a good going over to see what attributes do and don't make sense, really.
-- Tom
added comments: https://lists.denx.de/pipermail/u-boot/2023-March/513078.html
On Thu, Mar 23, 2023 at 1:31 PM Tom Rini trini@konsulko.com wrote:
On Thu, Mar 23, 2023 at 01:29:29PM -0700, appujee wrote:
So, saying "unused" and then "used" doesn't seem to make any sense.
unused and used attributes do not cancel each other. They have different semantics. I agree this part of the code needs some attention. zero sized arrays are not C compliant as I understand it, even more so when it is declared outside of a struct.
Ah, yeah, at least a comment explaining what and why we're doing what we're doing here, and probably some other parts of the linker list code that need addressing then, please.
On Wed, Mar 22, 2023 at 4:58 PM Tom Rini trini@konsulko.com wrote:
On Tue, Feb 21, 2023 at 03:33:20PM -0800, Aditya Kumar wrote:
The variable gets dropped by clang compiler in an optimized builds. Adding attribute((used)) allows the symbol to be preserved. Similar changes have been proposed in the past e.g., 569524741a01e1a96fc2b75dd7e5d12e41ce6c2b for ll_entry_declare macro.
Signed-off-by: AdityaK appujee@google.com Reviewed-by: Simon Glass sjg@chromium.org
include/linker_lists.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linker_lists.h b/include/linker_lists.h index d3da9d44e8..4cd13c3bc8 100644 --- a/include/linker_lists.h +++ b/include/linker_lists.h @@ -125,7 +125,7 @@ #define ll_entry_start(_type, _list) \ ({ \ static char start[0] __aligned(CONFIG_LINKER_LIST_ALIGN) \
- __attribute__((unused)) \
- __attribute__((unused)) __attribute__((used)) \ __section("__u_boot_list_2_"#_list"_1"); \ (_type *)&start; \
})
So, saying "unused" and then "used" doesn't seem to make any sense. And given some other problems we see with newer clang, which Simon reports this patch doesn't fully fix, we probably need to give that area a good going over to see what attributes do and don't make sense, really.
-- Tom
-- Tom
On Thu, Mar 23, 2023 at 03:38:33PM -0700, appujee wrote:
added comments: https://lists.denx.de/pipermail/u-boot/2023-March/513078.html
OK, and that references a gcc bugzilla entry where Andrew Pinski suggest a change to correct the code, and says not to do what you're suggesting here. And since there's still issues (as seen when moving to clang-15), I want to see a patch that does what he suggests instead please, thanks.
On Thu, Mar 23, 2023 at 1:31 PM Tom Rini trini@konsulko.com wrote:
On Thu, Mar 23, 2023 at 01:29:29PM -0700, appujee wrote:
So, saying "unused" and then "used" doesn't seem to make any sense.
unused and used attributes do not cancel each other. They have different semantics. I agree this part of the code needs some attention. zero sized arrays are not C compliant as I understand it, even more so when it is declared outside of a struct.
Ah, yeah, at least a comment explaining what and why we're doing what we're doing here, and probably some other parts of the linker list code that need addressing then, please.
On Wed, Mar 22, 2023 at 4:58 PM Tom Rini trini@konsulko.com wrote:
On Tue, Feb 21, 2023 at 03:33:20PM -0800, Aditya Kumar wrote:
The variable gets dropped by clang compiler in an optimized builds. Adding attribute((used)) allows the symbol to be preserved. Similar changes have been proposed in the past e.g., 569524741a01e1a96fc2b75dd7e5d12e41ce6c2b for ll_entry_declare macro.
Signed-off-by: AdityaK appujee@google.com Reviewed-by: Simon Glass sjg@chromium.org
include/linker_lists.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linker_lists.h b/include/linker_lists.h index d3da9d44e8..4cd13c3bc8 100644 --- a/include/linker_lists.h +++ b/include/linker_lists.h @@ -125,7 +125,7 @@ #define ll_entry_start(_type, _list) \ ({ \ static char start[0] __aligned(CONFIG_LINKER_LIST_ALIGN) \
- __attribute__((unused)) \
- __attribute__((unused)) __attribute__((used)) \ __section("__u_boot_list_2_"#_list"_1"); \ (_type *)&start; \
})
So, saying "unused" and then "used" doesn't seem to make any sense. And given some other problems we see with newer clang, which Simon reports this patch doesn't fully fix, we probably need to give that area a good going over to see what attributes do and don't make sense, really.
-- Tom
-- Tom
I think the correct fix is to not use a zero sized array at all.
AIUI, what Andrew Pinksi intended was: the `asm` fix and attribute(unused) fix aren't the same thing.
Using either way to get around this is probably not going to work in future; but I'm not familiar with the codebase to do large changes.
On Thu, Mar 23, 2023 at 3:44 PM Tom Rini trini@konsulko.com wrote:
On Thu, Mar 23, 2023 at 03:38:33PM -0700, appujee wrote:
added comments: https://lists.denx.de/pipermail/u-boot/2023-March/513078.html
OK, and that references a gcc bugzilla entry where Andrew Pinski suggest a change to correct the code, and says not to do what you're suggesting here. And since there's still issues (as seen when moving to clang-15), I want to see a patch that does what he suggests instead please, thanks.
On Thu, Mar 23, 2023 at 1:31 PM Tom Rini trini@konsulko.com wrote:
On Thu, Mar 23, 2023 at 01:29:29PM -0700, appujee wrote:
So, saying "unused" and then "used" doesn't seem to make any sense.
unused and used attributes do not cancel each other. They have different semantics. I agree this part of the code needs some attention. zero sized arrays are not C compliant as I understand it, even more so when it is declared outside of a struct.
Ah, yeah, at least a comment explaining what and why we're doing what we're doing here, and probably some other parts of the linker list code that need addressing then, please.
On Wed, Mar 22, 2023 at 4:58 PM Tom Rini trini@konsulko.com wrote:
On Tue, Feb 21, 2023 at 03:33:20PM -0800, Aditya Kumar wrote:
The variable gets dropped by clang compiler in an optimized builds. Adding attribute((used)) allows the symbol to be preserved. Similar changes have been proposed in the past e.g., 569524741a01e1a96fc2b75dd7e5d12e41ce6c2b for ll_entry_declare macro.
Signed-off-by: AdityaK appujee@google.com Reviewed-by: Simon Glass sjg@chromium.org
include/linker_lists.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linker_lists.h b/include/linker_lists.h index d3da9d44e8..4cd13c3bc8 100644 --- a/include/linker_lists.h +++ b/include/linker_lists.h @@ -125,7 +125,7 @@ #define ll_entry_start(_type, _list) \ ({ \ static char start[0] __aligned(CONFIG_LINKER_LIST_ALIGN) \
- __attribute__((unused)) \
- __attribute__((unused)) __attribute__((used)) \ __section("__u_boot_list_2_"#_list"_1"); \ (_type *)&start; \
})
So, saying "unused" and then "used" doesn't seem to make any sense. And given some other problems we see with newer clang, which Simon reports this patch doesn't fully fix, we probably need to give that area a good going over to see what attributes do and don't make sense, really.
-- Tom
-- Tom
-- Tom
participants (4)
-
Aditya Kumar -
appujee
-
Simon Glass -
Tom Rini