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

Feature/update tbb 2020 2 #121

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

SteveBronder
Copy link

@SteveBronder SteveBronder commented Jun 8, 2020

Howdy!

This just runs the script in tbb-update.R for TBB 2020.2 from the travis logs of #111 it looks like there was something weird with the suppress_warnings header in 2020.1 that is fixed in 2020.2

In file included from ../../src/tbb/concurrent_hash_map.cpp:17:0:
../../include/tbb/concurrent_hash_map.h:21:54:
    fatal error: internal/_warning_suppress_enable_notice.h: No such file or directory

From #111

Still need to port Windows-specific fixes.
Note: this unfortunately does not fix ASAN / UBSAN warnings.

I don't have access to a windows machine (linux/chrome-os only). Is wine the way to go for testing the windows specific patches?

@SteveBronder
Copy link
Author

SteveBronder commented Jun 8, 2020

This still has two warnings

../../src/tbbmalloc/proxy.cpp:343:6: warning: the program should also define ‘void operator delete(void*, std::size_t)’ [-Wsized-deallocation]
  343 | void operator delete(void* ptr) __TBB_NO_THROW {
      |      ^~~~~~~~
../../src/tbbmalloc/proxy.cpp:347:6: warning: the program should also define ‘void operator delete [](void*, std::size_t)’ [-Wsized-deallocation]
  347 | void operator delete[](void* ptr) __TBB_NO_THROW {

From cppreference tho'

(signatures) Called instead of (1-2) if a user-defined replacement is provided, except that it's unspecified whether (1-2) or (5-6) is called when deleting objects of incomplete type and arrays of non-class and trivially-destructible class types. A memory allocator can use the given size to be more efficient. The standard library implementations are identical to (1-2 [Steve: The non-size_t signatures]).

So if we want to patch those warnings they can just be defined as

void operator delete(void* ptr, size_t sz) __TBB_NO_THROW {
    InitOrigPointers();
    __TBB_malloc_safer_free(ptr, (void (*)(void*))orig_free);
}

@SteveBronder
Copy link
Author

ping @kevinushey

@SteveBronder
Copy link
Author

@kevinushey looking over the new version of the tbb it looks like they handle some the memset things that you had to manually patch in before. Is there anything I can help with to make this review easier?

We've been looking at github actions for rstan and those could potentially help sanity check some of the different OS/compiler versions here as well

https://github.com/stan-dev/rstan/pull/843/files

@SteveBronder
Copy link
Author

One thing to note here is that I had a hard time configuring the files in old to compile. But you also define -DTBB_NO_LEGACY=1 in the CXXFLAGS so I think it would be safe to delete the line here in makefile.tbb

# OLD/Legacy object files for backward binary compatibility
ifeq (,$(findstring $(DEFINE_KEY)TBB_NO_LEGACY,$(CPLUS_FLAGS)))
TBB_CPLUS_OLD.OBJ = \
		concurrent_vector_v2.$(OBJ) \
		concurrent_queue_v2.$(OBJ) \
		spin_rw_mutex_v2.$(OBJ) \
		task_v2.$(OBJ)
endif

I've looked over a good bit of the changes and it seems like they have much better support for mingw / windows now

@kevinushey
Copy link
Contributor

Thanks for continuing to work on this PR, and sorry for taking so long to respond -- it's been challenging to find time! If you can get the Linux + macOS work into a good state, I'll try to find time to get the Windows bits working as well.

I'm on board with removing old/ as well.

@SteveBronder
Copy link
Author

Totally no worries! Do you have a script to run all the reverse dependencies? I think if we have that I can steal some stuff from rstan to run a github actions script to run the tests and reverse dependencies on mac/windows/linux

@kevinushey
Copy link
Contributor

I don't; I usually just run reverse dependency checks with revdepcheck.

@Enchufa2
Copy link
Member

FWIW, I've been building RcppParallel against the system tbb v2020 here, and I've detected no issues from any dependency.

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.

3 participants