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

CA-407107: makefile fixes to honour LDFLAGS #23

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

edwintorok
Copy link
Contributor

No description provided.

xha.spec does a 'make install' which installs the debug build by default.
The only difference between debug and release is NDEBUG (no -O flags are used),
but considering that we've always shipped the debug version in production builds,
keep that one.

There are some advantages to shipping the debug version:
* stacktraces may work better than at O2
* if the assertions finds a bug in the code it might be better to stop, 
  rather than continue and do something incorrect (corrupt memory, etc.).
  Given that in the past years that we've been shipping XHA with asserts on we haven't really found
  failed assertions, keep them on.

Signed-off-by: Edwin Török <[email protected]>
The GCC manual recommends this over O0 to improve the debugging experience:

> It is a better choice than -O0 for producing debuggable code because some compiler passes
> that collect debug information are disabled at -O0.:w

It is also better than -O2 that rpmbuild would use because according to the GCC manual:
> To get more accurate stack traces, it is possible to use options such as -O0, -O1, or -Og (which, for instance, prevent most function inlining), -fno-optimize-sibling-calls (which prevents optimizing sibling and tail recursive calls; this option is implicit for -O0, -O1, or -Og), or -fno-ipa-icf (which disables Identical Code Folding for functions)

`-ipa-icf` is only enabled at -O2 and above, so using -Og here should be fine.

(stacktraces are printed by log.c)

Signed-off-by: Edwin Török <[email protected]>
Add it to LIBS at the end.

Signed-off-by: Edwin Török <[email protected]>
Stacktraces are printed by log.c, to make them better use -Og which prevents some optimizations
that would alter stacktraces significantly.

Signed-off-by: Edwin Török <[email protected]>
Parallel builds can be enabled by using `$(MAKE)` instead of `make`.

However the current rules have race conditions and do not fully declare their dependencies:
* the .a rule was producing an archive out of whatever .o files it could fine at the time it ran, and it also deleted the files it produced
* the .a rule always rebuilt all the files
* the .a rule produced some file that were also produced by the %.o: %.c rule, leading to a race condition

Move the .a rule to the lib subdirectory, and explicitly specify dependencies and only use files during a build that have been declared as
a dependency of a rule.

Also enable incremental builds by removing the PHONY from buildid.h.
During an RPM build we'll get a clean build and redefined buildid anyway.

Signed-off-by: Edwin Török <[email protected]>
@edwintorok
Copy link
Contributor Author

This will depend on some of my other warning fixes, because some of these warnings are now fatal.

@edwintorok edwintorok marked this pull request as draft February 21, 2025 20:40
Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

Unrelated but would suggest to add a format target to avoid the problems of inconsistent indentation.

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

Successfully merging this pull request may close these issues.

2 participants