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

Feat/weak attribute #47

Closed
wants to merge 2 commits into from

Conversation

susundberg
Copy link

Hi!

Here is pull request for issue #43. I have tested it only with gcc (i guess clang will be tested with the CI) - and used in my own project.

@louiske0911
Copy link

Could I ask you why does need the attribute((weak)) symbol ?

@susundberg
Copy link
Author

Hi! I tried to reason the need in issue 43 .

In short, i have a set of files (generated), that define fake functions for all real functions. This is compiled to be library -- say "libfake.a". Now when linking a test binary, i have maybe one or two real functions, and this binary is linked against the fakelib (i have many of those binaries - one for each set of real functions!). Now without the weak one gets linker error -- the real function is defined also in fakelib. With weak linker is instructed to select the other one with no error.

I could surely pick the files for compiling, but that would require additional "makefile"-logic -- now i can compile the whole fakelib once, and let the linker select suitable implementation for required functions.

@meekrosoft
Copy link
Owner

meekrosoft commented Aug 8, 2018

I am a bit reluctant to merge this one...
I completely see the need - but not every compiler supports weak attributes using this syntax so it would break some users. Is there another way? Perhaps a preprocessor switch? I imagining adding a helper macro to output something like this:


#IFDEF GCC_WEAK_FUNCTIONS
   #DEFINE WEAK_OPTION __attribute__((weak))
#ELSE
    #DEFINE WEAK_OPTION 
#
"#{return_type} WEAK_OPTION FUNCNAME(#{arg_val_list(arg_count)}#{varargs})"

@meekrosoft
Copy link
Owner

Then users could include fff.h to turn on the option:

#DEFINE FFF_GCC_WEAK_FUNCTIONS
#include "fff.h"

@wulfgarpro
Copy link
Collaborator

@susundberg, please resolve your conflicts before review moves forward.

@AppVeyorBot
Copy link

Build fff 23-appveyor failed (commit a90dd84fe2 by @susundberg)

@AppVeyorBot
Copy link

@susundberg
Copy link
Author

susundberg commented Dec 4, 2018

Hi! Sorry for long delay.

I (force) pushed new update; i replaced the hardcoded __attribute__(weak) with #ifdef FFF_FUNCTION_ATTRIBUTE as i though it would fit better.

I updated also the generated fff.h but there seems to be whitespace differences also on unmodified master. I am running ubuntu 18.04 with

ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux-gnu]

Any suggestions what version to use (and maybe add that to readme?).

@wulfgarpro
Copy link
Collaborator

@susundberg, the only whitespace changes I can see in your PR are good ones. It looks like you've used your editor to strip them.

Copy link
Collaborator

@wulfgarpro wulfgarpro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@susundberg, before I go ahead and do anything, can you please add some test cases?

@susundberg
Copy link
Author

Hi! Sorry for the delay again. I actually did checked out the tests before making the pull request but decided not to go there since it was not clear me how they are structured etc.

I now went back to look how it goes - and would an examples/weak_linking/ with some tests included be sufficient?

Happy holidays!

@wulfgarpro
Copy link
Collaborator

wulfgarpro commented Dec 20, 2018 via email

@AppVeyorBot
Copy link

@susundberg
Copy link
Author

Uh sorry, ignore, force pushed after pulling to latest, examples still missing ..

…attributes for functions. More specifically, allow __weak__ attribute.
@AppVeyorBot
Copy link

Build fff 42-appveyor completed (commit 20af33657d by @)

@AppVeyorBot
Copy link

Build fff 43-appveyor completed (commit 767dbfaa3f by @)

@AppVeyorBot
Copy link

Build fff 44-appveyor completed (commit 7818963220 by @)

@AppVeyorBot
Copy link

Build fff 45-appveyor completed (commit cdb6a8c5c2 by @)

@susundberg
Copy link
Author

Hi all again. I took a while but i added an example "weak_linking". Its compiled and runned on the normal command. The example itself is silly - as you can guess - but the Makefile includes three example cases where weak linking is required.

I did not make any (automatic) check that the compiling fails if the weak linking flag is not set. I did try manually though.

How does it look?

@@ -26,6 +26,13 @@ def output_constants
putd "#define FFF_CALL_HISTORY_LEN (#{$MAX_CALL_HISTORY}u)"
}
putd "#endif"
# If user has defined functions to have some attributes, use those. If not, define as empty so it wont affect others.
putd "#ifndef FFF_FUNCTION_ATTRIBUTES"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather this be named:

FFF_GCC_FUNCTION_ATTRIBUTES

wulfgarpro added a commit that referenced this pull request Feb 3, 2019
commit e5a5749
Author: James Fraser <wulfgar.pro@gmail.com>
Date:   Sun Feb 3 19:57:31 2019 +1100

    PR #47: Minor review fixes to tests files.

commit e9f11b9
Author: James Fraser <wulfgar.pro@gmail.com>
Date:   Sun Feb 3 19:57:04 2019 +1100

    PR #47: Minor review fixes.

commit 0a7fbec
Author: Pauli Salmenrinne <pauli.salmenrinne@pexraytech.com>
Date:   Tue Jan 22 15:11:10 2019 +0200

    Add example for the weak linking

commit 6477373
Author: susundberg <susundberg@gmail.com>
Date:   Wed Mar 21 13:14:05 2018 +0200

    Add "FFF_FUNCTION_ATTRIBUTES" definition that can be used to declare attributes for functions. More specifically, allow __weak__ attribute.
@wulfgarpro wulfgarpro closed this Feb 3, 2019
@wulfgarpro
Copy link
Collaborator

@susundberg, see merge 0b9e9f5.

Thank you for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants