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.

Sign up for GitHub

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

Jump to bottom

Create proper debug configuration #6193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Create proper debug configuration #6193

wants to merge 1 commit into from

Conversation

icegood
Copy link

@icegood icegood commented Aug 13, 2024

With this PR it is easier to create more rigorous debug builds for further debugging in gdb.
(for at least spk/ffmpeg)
Not applicable out of the box since other dependant repos need to be fit as well.

To check against
https://github.com/icegood/spksrc/tree/fix/libpng_debug_builds at it fixes png bug

Applicable for issue #6176

Description

Fixes #

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • [x ] Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

@icegood
Create proper debug configuration
5b303d0 
for at least spk/ffmpeg

To check against
https://github.com/icegood/spksrc/tree/fix/libpng_debug_builds
at it fixes png bug

Applicable for issue SynoCommunity#6176

Signed-off-by: Serhii Ivanov <icegood1980@gmail.com>
th0ma7
th0ma7 requested changes Aug 17, 2024
View reviewed changes
Copy link
Contributor

@th0ma7 th0ma7 left a comment

Choose a reason for hiding this comment

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

First off, thnx a lot for your contribution. You actually went quite a bit into the weeds in here, well done. You'll find a a few questions, suggestions & changes proposed part of my review.

cross/ffmpeg6/Makefile
Comment on lines -13 to -15
CONFIGURE_ARGS += --extra-cflags="-I$(WORK_DIR)/install$(INSTALL_PREFIX)/include"
CONFIGURE_ARGS += --extra-ldflags="-L$(WORK_DIR)/install$(INSTALL_PREFIX)/lib"
CONFIGURE_ARGS += --extra-libs="-lxml2 -ldl -lm" --pkg-config=/usr/bin/pkg-config --ranlib=$(RANLIB)
Copy link
Contributor

Choose a reason for hiding this comment

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

This code dates way-back, before I started maintaining ffmpeg. Why is only pkg-config related code kept?

cross/x264/Makefile
@@ -19,7 +19,6 @@ GNU_CONFIGURE = 1
CONFIGURE_ARGS = --enable-shared
CONFIGURE_ARGS += --enable-pic
CONFIGURE_ARGS += --disable-opencl
CONFIGURE_ARGS += --prefix=$(INSTALL_PREFIX)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the prefix being removed? Presumably because the spksrc.configure.mk adds it anyway?

mk/spksrc.cross-cmake-env.mk
Comment on lines +10 to +12
CMAKE_ARGS += -DCMAKE_BUILD_TYPE=Debug
endif
CMAKE_ARGS += -DCMAKE_BUILD_TYPE=Release
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be ifeq / else / endif such as:

ifeq ($(strip $(CMAKE_BUILD_TYPE)),)
  ifeq ($(strip $(GCC_DEBUG_INFO)),1)
    CMAKE_ARGS += -DCMAKE_BUILD_TYPE=Debug
  else
    CMAKE_ARGS += -DCMAKE_BUILD_TYPE=Release
  endif
else
CMAKE_ARGS += -DCMAKE_BUILD_TYPE=$(CMAKE_BUILD_TYPE)
endif

mk/spksrc.cross-env.mk
Comment on lines +24 to +36
ADDITIONAL_CFLAGS := $(patsubst -O%,,$(ADDITIONAL_CFLAGS))
ADDITIONAL_CPPFLAGS := $(patsubst -O%,,$(ADDITIONAL_CPPFLAGS))
ADDITIONAL_CXXFLAGS := $(patsubst -O%,,$(ADDITIONAL_CXXFLAGS))
ifeq ($(strip $(GCC_DEBUG_INFO)),1)
GCC_DEBUG_FLAGS = -O0 -g3
ADDITIONAL_CFLAGS := $(patsubst -O%,,$(ADDITIONAL_CFLAGS)) $(GCC_DEBUG_FLAGS)
ADDITIONAL_CPPFLAGS := $(patsubst -O%,,$(ADDITIONAL_CPPFLAGS)) $(GCC_DEBUG_FLAGS)
ADDITIONAL_CXXFLAGS := $(patsubst -O%,,$(ADDITIONAL_CXXFLAGS)) $(GCC_DEBUG_FLAGS)
# other options to consider:
#GCC_DEBUG_FLAGS += -fsanitize=address -fsanitize=undefined -fstack-protector-all
GCC_DEBUG_FLAGS = -ggdb3 -g3
else
GCC_DEBUG_FLAGS = -O3 -fomit-frame-pointer
endif
ADDITIONAL_CFLAGS := $(ADDITIONAL_CFLAGS) $(GCC_DEBUG_FLAGS)
ADDITIONAL_CPPFLAGS := $(ADDITIONAL_CPPFLAGS) $(GCC_DEBUG_FLAGS)
ADDITIONAL_CXXFLAGS := $(ADDITIONAL_CXXFLAGS) $(GCC_DEBUG_FLAGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

This has a much broader effect than only affecting debug builds. This is turn will enforce using -O3 -fomit-frame-pointer on all buids, which will not work due to some arch specific caveats that requires special flags to be used. GCC_DEBUG_FLAGS should only be used for GCC_DEBUG_INFO builds and not be affecting other regular builds.

Also I believe this sort of explains the reasons for removing -O? in some other Makefiles?

mk/spksrc.tc.mk
Comment on lines -156 to 161
echo TC_ENV += CFLAGS=\"$(CFLAGS) $(GCC_DEBUG) $$\(ADDITIONAL_CFLAGS\)\" ; \
echo TC_ENV += CPPFLAGS=\"$(CPPFLAGS) $(GCC_DEBUG) $$\(ADDITIONAL_CPPFLAGS\)\" ; \
echo TC_ENV += CXXFLAGS=\"$(CXXFLAGS) $(GCC_DEBUG) $$\(ADDITIONAL_CXXFLAGS\)\" ; \
echo TC_ENV += CFLAGS=\"$(CFLAGS) $$\(ADDITIONAL_CFLAGS\)\" ; \
echo TC_ENV += CPPFLAGS=\"$(CPPFLAGS) $$\(ADDITIONAL_CPPFLAGS\)\" ; \
echo TC_ENV += CXXFLAGS=\"$(CXXFLAGS) $$\(ADDITIONAL_CXXFLAGS\)\" ; \
echo TC_ENV += LDFLAGS=\"$(LDFLAGS) $$\(ADDITIONAL_LDFLAGS\)\" ; \
echo TC_ENV += CARGO_HOME=\"$(realpath $(CARGO_HOME))\" ; \
echo TC_ENV += RUSTUP_HOME=\"$(realpath $(RUSTUP_HOME))\" ; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't thig be GCC_DEBUG_FLAGS?

mk/spksrc.tc.mk
Comment on lines 170 to 178
echo TC_TARGET := $(TC_TARGET) ; \
echo TC_PREFIX := $(TC_PREFIX) ; \
echo TC_PATH := $(WORK_DIR)/$(TC_TARGET)/bin/ ; \
echo CFLAGS := $(CFLAGS) $(GCC_DEBUG) $$\(ADDITIONAL_CFLAGS\) ; \
echo CPPFLAGS := $(CPPFLAGS) $(GCC_DEBUG) $$\(ADDITIONAL_CPPFLAGS\) ; \
echo CXXFLAGS := $(CXXFLAGS) $(GCC_DEBUG) $$\(ADDITIONAL_CXXFLAGS\) ; \
echo CFLAGS := $(CFLAGS) $$\(ADDITIONAL_CFLAGS\) ; \
echo CPPFLAGS := $(CPPFLAGS) $$\(ADDITIONAL_CPPFLAGS\) ; \
echo CXXFLAGS := $(CXXFLAGS) $$\(ADDITIONAL_CXXFLAGS\) ; \
echo LDFLAGS := $(LDFLAGS) $$\(ADDITIONAL_LDFLAGS\) ; \
echo TC_INCLUDE := $(TC_INCLUDE) ; \
echo TC_LIBRARY := $(TC_LIBRARY) ; \
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise to previous comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@th0ma7 th0ma7 th0ma7 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@icegood @th0ma7