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

make/debug #853

Closed
wants to merge 4 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@yizhang-cae
Contributor

yizhang-cae commented Apr 30, 2018

Submission Checklist

  • Run unit tests: ./runTests.py test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary:

Add DEBUG=1 option to make to allow debug build with CXXFLAGS +=-DDEBUG -g -O0

Intended Effect:

This has been sitting in my local but I guess it helps that developers use consistent flags to share debug results.

As to using -O0 vs -Og, see
https://stackoverflow.com/questions/12970596/gcc-4-8-does-og-imply-g/27076307#27076307

We can propagate the setup to downstream repos later on.

How to Verify:

use #ifdef DEBUG to insert debug codes, and make with

make DEBUG=1 foo

Maybe we also want to also unit test with debug build. I'll leave it to general discussion.

Side Effects:

N/A

Documentation:

N/A

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Metrum Research Group, LLC

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@bob-carpenter

This comment has been minimized.

Contributor

bob-carpenter commented May 1, 2018

Is the plan to merge code into the development branch with #ifdef DEBUG or is this for temporary use on branches?

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented May 1, 2018

@syclik

This comment has been minimized.

Member

syclik commented May 1, 2018

I restarted it, but I think this may be a legitimate failure due to an interaction with MPI.

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented May 2, 2018

Seems passed. I don't see how this would interfere with mpi build.

@syclik

This comment has been minimized.

Member

syclik commented May 11, 2018

@yizhang-cae, it really doesn't work, as far as I know. Any ideas why not?

I think the idea of this is good.

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented May 11, 2018

@syclik

This comment has been minimized.

Member

syclik commented May 24, 2018

@yizhang-cae, any luck?

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented May 24, 2018

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented May 31, 2018

@syclik, I've fixed it.

@syclik

Thanks for updating. Changes requested. Not sure what's the deal with the additional lines to make/tests-- just some justification or removal would help.

make/debug Outdated
@@ -0,0 +1,7 @@
DEBUG ?= 0
ifeq ($(DEBUG), 1)
CXXFLAGS:=$(filter-out -O${O},$(CXXFLAGS))

This comment has been minimized.

@syclik

syclik May 31, 2018

Member

Would you mind being consistent with formatting? It'd be nice if it was something like:

CXXFLAGS := $(filter-out -O${O}, $(CXXFLAGS))

(spaces around the operator and the space after the comma).

make/debug Outdated
DEBUG ?= 0
ifeq ($(DEBUG), 1)
CXXFLAGS:=$(filter-out -O${O},$(CXXFLAGS))
GTEST_CXXFLAGS:=$(filter-out -O${O},$(GTEST_CXXFLAGS))

This comment has been minimized.

@syclik

syclik May 31, 2018

Member

also related to spacing, replace these tabs with two spaces, like the changes below. (I don't know off the top of my head if that's consistent with the rest of the makefile... go with what's more consistent.)

make/tests Outdated
@@ -40,6 +40,14 @@ test/unit/libmultiple.so : test/unit/multiple_translation_units1.o test/unit/mul
test/unit/multiple_translation_units_test.cpp : test/unit/libmultiple.so
DEBUG_TESTS := $(subst .cpp,$(EXE),$(shell find test -name *debug_*test.cpp))

This comment has been minimized.

@syclik

syclik May 31, 2018

Member

What is this? It wasn't included in the description of the pull request. It seems redundant?

This comment has been minimized.

@yizhang-cae

yizhang-cae May 31, 2018

Contributor

In case in the future we want do something special of some diagnostic/debug tests. Single them out and change/add flags.

This comment has been minimized.

@syclik

syclik May 31, 2018

Member

Let's remove this if it's for unspecified future use.

This comment has been minimized.

@yizhang-cae

yizhang-cae May 31, 2018

Contributor

Done

make/tests Outdated
@@ -40,6 +40,14 @@ test/unit/libmultiple.so : test/unit/multiple_translation_units1.o test/unit/mul
test/unit/multiple_translation_units_test.cpp : test/unit/libmultiple.so
DEBUG_TESTS := $(subst .cpp,$(EXE),$(shell find test -name *debug_*test.cpp))

This comment has been minimized.

@yizhang-cae

yizhang-cae May 31, 2018

Contributor

In case in the future we want do something special of some diagnostic/debug tests. Single them out and change/add flags.

@syclik

syclik approved these changes May 31, 2018

@syclik

Sorry -- I missed that there was no accompanying documentation. Can you post directions on the Stan Math wiki and/or the makefile help target itself?

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented May 31, 2018

Added comments in make/debug. Wiki page on the way.

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented May 31, 2018

@syclik

This comment has been minimized.

Member

syclik commented May 31, 2018

@syclik

syclik approved these changes May 31, 2018

Thanks!

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented May 31, 2018

@yizhang-cae

This comment has been minimized.

Contributor

yizhang-cae commented Jun 11, 2018

The MPI failures don't make much sense to me. Anyways, this is not critical. I'll leave it for future revisit.

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