-
Notifications
You must be signed in to change notification settings - Fork 38
rhel:Correct the usage of __attribute__ #287
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @zhangzikang01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical compatibility issue in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request correctly addresses an issue with GCC attribute usage for symbol visibility. The analysis in the description is thorough and the fix is applied consistently across the codebase. By moving the PART_OF_API macro before the return type in function definitions, you've ensured that the functions are correctly exported in the shared library, which is crucial for compatibility. The removal of the pragma to suppress attribute warnings is also a good cleanup, as the root cause of the warning is now fixed. I've added one minor style suggestion for consistency.
Signed-off-by: zhangzikang01 <zhangzikang@kylinos.cn>
Signed-off-by: zhangzikang01 zhangzikang@kylinos.cn
Comparison of the librelp-1.2.16 version reveals that the librelp.so.0 library generated by compiling the current version of the source code no longer includes the relpEngineGetVersion symbol, which does not meet compatibility requirements. Therefore, an analysis of the source code was conducted.
The analysis found that in the Makefile.am file, "AM_CFLAGS=-fvisibility=hidden" was added, setting the default attribute to hidden. Then, during function definition, the attribute was modified to default via "attribute((visibility("default")))". However, modifying the attribute to default at the definition location of the relpEngineGetVersion function did not take effect. The relpEngineGetVersion function retained the hidden attribute, resulting in the absence of the relpEngineGetVersion symbol in the compiled librelp.so.0 library.
Consult the information in the following link:
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
The material indicates that for functions, the attribute should be placed either before the declaration type or at the very end of the function.
A test demo was written to verify the usage of attribute:
#include <stdio.h>
#define PART_OF_API attribute((visibility("default")))
#define VERSION "1.11.0"
#define CH 't'
const char * test_0(void)
{
return (char*) (VERSION);
}
const char * PART_OF_API test_1(void)
{
return (char*) (VERSION);
}
PART_OF_API const char * test_2(void)
{
return (char*) (VERSION);
}
void main()
{
printf("test!\n");
}
gcc -fvisibility=hidden test.c -o libtest.o
test.c:13:1: warning: the 'visibility' attribute is ignored on a non-class type [-Wattributes]
13 | {
| ^
readelf -sW libtest.o | grep test_
Based on the test results, it was found that:
(1) Without adding attribute((visibility("default"))): the attribute remains hidden.
(2) Adding attribute((visibility("default"))) before the declaration type: the attribute is successfully modified to default.
(3) Adding attribute((visibility("default"))) between the declaration type and the function name: the attribute modification does not take effect, remains hidden, and a warning is generated (though warning messages are suppressed in the source code: #pragma GCC diagnostic ignored "-Wattributes").
Therefore, it is preliminarily determined that the incorrect placement of attribute((visibility("default"))) caused the attribute modification to fail. Hence, this PR is submitted to address the issue.