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

RecoTracker/TkDetLayers: resolve link issue with Clang #7259

Merged

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented Jan 18, 2015

The following fix is required due to a behavior difference between Clang
and GCC. Clan does not correctly emulate GCC visibility pragma.

See PR22254: http://llvm.org/bugs/show_bug.cgi?id=22254

The link error involved the following two symbols:

  • Plane::tangentPlane(Point3DBase<float, LocalTag> const&) const
  • Plane::tangentPlane(Point3DBase<float, GlobalTag> const&) const

On link command we have libTrackingToolsDetLayers.so and
libTrackingToolsGeomPropagators.so, which have undefined references
for above two symbols. These are usually resolved on dynamic loading.

On Clang TECLayer.o marked these particular symbols as HIDDEN.

Clang:

  182: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND
_ZNK5Plane12tangentPlaneERK11Point3DBaseIf8LocalTagE
  183: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND
_ZNK5Plane12tangentPlaneERK11Point3DBaseIf9GlobalTagE

GCC:

  261: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND
_ZNK5Plane12tangentPlaneERK11Point3DBaseIf9GlobalTagE
  262: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND
_ZNK5Plane12tangentPlaneERK11Point3DBaseIf8LocalTagE

TkDetUtil.h inside visibility pragma section has forward declaration
of class Plane. Thus Clang marked it as HIDDEN. Then linker was
complaining that two libraries mentioned above reference a HIDDEN
symbols, which is not allowed as these sybmols are not public API.

In GCC Plane is with DEFAULT visbility, because the first declaration
wins, thus the DEFAULT visibility. You cannot modify it later on.

Lesson here: do not add forward declaration for publicly available
classes in pragma visibility hidden section.

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

The following fix is required due to a behavior difference between Clang
and GCC. Clan does not correctly emulate GCC visibility pragma.

See PR22254: http://llvm.org/bugs/show_bug.cgi?id=22254

The link error involved the following two symbols:
- `Plane::tangentPlane(Point3DBase<float, LocalTag> const&) const`
- `Plane::tangentPlane(Point3DBase<float, GlobalTag> const&) const`

On link command we have `libTrackingToolsDetLayers.so` and
`libTrackingToolsGeomPropagators.so`, which have undefined references
for above two symbols. These are usually resolved on dynamic loading.

On Clang `TECLayer.o` marked these particular symbols as HIDDEN.

Clang:

  182: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND
_ZNK5Plane12tangentPlaneERK11Point3DBaseIf8LocalTagE
  183: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND
_ZNK5Plane12tangentPlaneERK11Point3DBaseIf9GlobalTagE

GCC:

  261: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND
_ZNK5Plane12tangentPlaneERK11Point3DBaseIf9GlobalTagE
  262: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND
_ZNK5Plane12tangentPlaneERK11Point3DBaseIf8LocalTagE

`TkDetUtil.h` inside visibility pragma section has forward declaration
of class `Plane`. Thus Clang marked it as HIDDEN. Then linker was
complaining that two libraries mentioned above reference a HIDDEN
symbols, which is not allowed as these sybmols are not public API.

In GCC `Plane` is with DEFAULT visbility, because the first declaration
wins, thus the DEFAULT visibility. You cannot modify it later on.

Lesson here: do not add forward declaration for publicly available
classes in pragma visibility hidden section.

Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @davidlt for CMSSW_7_4_X.

RecoTracker/TkDetLayers: resolve link issue with Clang

It involves the following packages:

RecoTracker/TkDetLayers

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @appeltel, @gpetruc, @cerati, @dgulhan, @venturia this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.

@davidlt
Copy link
Contributor Author

davidlt commented Jan 18, 2015

It might be good to give a look at all headers in this package, but that's not the scope of this PR.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@VinInn
Copy link
Contributor

VinInn commented Jan 19, 2015

+1
Thanks David to pinpoint such a subtle issue

@slava77
Copy link
Contributor

slava77 commented Jan 20, 2015

@ktf @nclopezo
Unrelated to the code here, but related to jenkins tests:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7259/1974/summary.html says that CMSSW_7_4_X_2015-01-17-1400 was used as a reference.
The topic branch for this PR, looking at the parent, is after #7255
(which is CMSSW_7_4_X_2015-01-18-0200).
So, the actual comparison is made between the baseline , and "baseline + all new features up to -18-0200 + this PR"
... not surprisingly we see changes in cosmic reconstruction.
It's good that it's obvious in this case.

How is the decision to select the baseline for comparisons made?
Could you please check the setup so that the diffs are actually made incrementally
Thank you.

@ktf
Copy link
Contributor

ktf commented Jan 20, 2015

The decision is done based on the availability of the baseline (which is different from the availability of the release, since tests need to be run). The alternative is to postpone the tests until a baseline for the release used for the test is there. I'll see what we can do once @nclopezo is back.

@ktf
Copy link
Contributor

ktf commented Jan 20, 2015

Merging since I take slava meant to +1 this and simply forgot.

ktf added a commit that referenced this pull request Jan 20, 2015
…tLayers-v2

RecoTracker/TkDetLayers: resolve link issue with Clang
@ktf ktf merged commit ac8e646 into cms-sw:CMSSW_7_4_X Jan 20, 2015
@cvuosalo
Copy link
Contributor

+1

for #7259 9d550f7

This PR includes a very small code change for a pragma directive that should have no effect in running programs.
The Jenkins tests were done on the wrong baseline and indicated spurious differences for workflow 4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC. The other Jenkins test results are OK.

The matrix tests on the correct baseline, CMSSW_7_4_X_2015-01-18-0200, show no discrepancies from the reference.

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

Successfully merging this pull request may close these issues.

None yet

6 participants