Project

General

Profile

Bug #18075

ABI types not available (again) after canvas/canvas_root_io refactoring

Added by Kyle Knoepfel almost 2 years ago. Updated 9 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Start date:
10/30/2017
Due date:
% Done:

100%

Estimated time:
Spent time:
Scope:
Internal
Experiment:
-
SSI Package:
Duration:

Description

Ben reports:

With the split of the ROOT part of canvas into a separate library, this issue has reared its head again. I've been able to resolve it using the techniques outlined in the LLVM/LibreOffice bug reports - canvas then compiles and all tests pass on macOS Sierra with Xcode 9. I can submit this patch, but first want to be sure on the testing.

However, looking at source:canvas/tests, I can't find explicit test cases for the functions in source:canvas/Persistency/Common/detail/maybeCastObj.h. It is used elsewhere, so I wanted to check if the tests exercise it sufficiently, or if further ones would be useful. In the latter case, I can implement these as part of the patch, but would need some guidance on the expected success/failure behaviour to check.


Related issues

Related to canvas - Bug #14005: ABI types not available in libc++/libc++abi for maybeCastObj Closed10/03/2016

History

#1 Updated by Kyle Knoepfel almost 2 years ago

  • Related to Bug #14005: ABI types not available in libc++/libc++abi for maybeCastObj added

#2 Updated by Kyle Knoepfel almost 2 years ago

We make use of maybeCastObj in integration tests of the art executable. However, we would be happy to accept any unit tests that you provide for this function. We tend to use the Boost unit test library for such things, but we also use standalone executables for the same purpose.

Regarding what behavior to test, their are several cases that are explicitly checked for in the implementations of the maybeCastObj function. The tests should verify the expected behavior spelled out in those cases. Look at canvas/Persistency/Common/{setPtr,getElementAddresses}.h for an example of its use.

#3 Updated by Ben Morgan over 1 year ago

I've attached a patch that provides a very experimental workaround for this issue (also browsable on GitHub: https://github.com/drbenmorgan/fnal-canvas/commit/d32ef80fa52330339fecd6a8dc5466de7fb9d4b1).

The commit message should hold all the main details on background, and links to the two main sources of information I used on this. Note that the patch is on top of Canvas v3_03_02 as that's the most current version I had dependencies available for (via the LArSoft CVMFS), but should apply fairly cleanly on newer tags or develop. The test maybeCastObj_t is pretty hacky and likely needs more cases, but all cases pass on the tested platforms:

- slf7 (actually CentOS7) with e15:debug
- slf7 with c2:debug

It still needs testing on Darwin in c2 mode, though I have tested on macOS natively with AppleClang (Apple LLVM version 9.1.0 (clang-902.0.39.2)) and this also works.
There is one AppleClang-ism in canvas/CMakeLists.txt which is to add libc++abi to the link list. That isn't needed on Linux/Clang as that library seems to get linked in automatically, so status of this on macOS with open-source Clang isn't clear just yet.

#4 Updated by Kyle Knoepfel about 1 year ago

  • Status changed from Accepted to Assigned
  • Assignee set to Kyle Knoepfel

Thanks, Ben, we'll take a look and let you know if we have any problems.

#5 Updated by Kyle Knoepfel about 1 year ago

  • Status changed from Assigned to Resolved
  • Target version set to Vega
  • % Done changed from 0 to 100

Except for minor conflicts, the patch applied cleanly. Since art does not yet support Apple Clang, we did remove the conditional dependency in the library link list.

This is a major step forward for art, Ben. We thank you for your contribution here--it is slated for the Vega release, which is likely to be the first release of the art 3.01 series.

#6 Updated by Kyle Knoepfel about 1 year ago

Resolved with commit canvas:abae2f8e.

#7 Updated by Kyle Knoepfel about 1 year ago

Turns out that the conditional library in the link list was necessary, even for open-source Clang:

- $<$<CXX_COMPILER_ID:AppleClang>:c++abi>
+ $<$<PLATFORM_ID:Darwin>:c++abi>

Implemented with commit canvas:6dfbc86.

#8 Updated by Kyle Knoepfel about 1 year ago

  • Target version changed from Vega to 3.01.00

#9 Updated by Kyle Knoepfel about 1 year ago

  • Target version changed from 3.01.00 to 3.02.00

#10 Updated by Kyle Knoepfel 9 months ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF