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

condformats_serialization_generate.py: support for aarch64 #3948

Merged
merged 2 commits into from May 22, 2014

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented May 21, 2014

libClang seems to act differently on AArch64 causing condformats_serialization_generate.py always to select 0 classes. It happens that is_definition() on AST node mostly returns false.

Even on such small examples:

$ cat my.h
struct timespec
{
  int tv_sec;
  int tv_nsec;
};

On Fedora 20 on x86_64

displayname: timespec, kind: CursorKind.STRUCT_DECL, is_definition: True,  location:<SourceLocation file 'my.h', line 1, column 8>
>> get_definition().location: <SourceLocation file 'my.h', line 1, column 8>

On Fedora 19 on aarch64

displayname: timespec, kind: CursorKind.STRUCT_DECL, is_definition: False,  location:<SourceLocation file 'my.h', line 1, column 8>
>> get_definition().location: <SourceLocation file 'my.h', line 1, column 8>

Change checks if node (declaration record) and it's definition location (file, line and column) match. If match is found, it's a definition.

Details from libClang side:

Breakpoint 1, clang_isCursorDefinition (C=...)
   at /home/david.abdurachmanov/new-arch/test/BUILD/fc19_aarch64_gcc490/external/llvm/3.4-cms2/llvm-3.4-6800b6d2afc/tools/clang/tools/libclang/CIndex.cpp:4709
4709      if (!clang_isDeclaration(C.kind))
(gdb) p C
$1 = {kind = CXCursor_ClassDecl, xdata = 0, data = {0x7fb39e60e0, 0x0, 0x7fb000cfb0}}
(gdb) p clang_getCString(clang_getCursorDisplayName(C))
$2 = 0x9a5cd0 "RunNumber"
(gdb) p C.
data   kind   xdata
(gdb) set $foo = clang_getCursorDefinition(C)
(gdb) p $foo
$3 = {kind = CXCursor_ClassDecl, xdata = 0, data = {0x7fb39e60e0, 0x1, 0x7fb000cfb0}}

As you can see C != clang_getCursorDefinition(C), because C.data[1] != clang_getCursorDefinition(C).data[1]

clang::cxcursor::operator== (X=..., Y=...)
   at /home/david.abdurachmanov/new-arch/test/BUILD/fc19_aarch64_gcc490/external/llvm/3.4-cms2/llvm-3.4-6800b6d2afc/tools/clang/tools/libclang/CXCursor.cpp:936
936       return X.kind == Y.kind && X.data[0] == Y.data[0] && X.data[1] == Y.data[1] &&
(gdb) info args
X = {kind = CXCursor_ClassDecl, xdata = 0, data = {0x7fb39e60e0, 0x1, 0x7fb000cfb0}}
Y = {kind = CXCursor_ClassDecl, xdata = 0, data = {0x7fb39e60e0, 0x0, 0x7fb000cfb0}}

data[1] seems to SourceLocation.ID.

I compared md5 hashes of Serialization.cc files using untouched and modified condformats_serialization_generate.py. All were identical.

This is my initial proposal until I understand better the difference in libClang behavior on aarch64 and x86_64, or it's fixed in upstream.

@cmsbuild
Copy link
Contributor

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

condformats_serialization_generate.py: support for aarch64

It involves the following packages:

CondFormats/Serialization

@ggovi, @cmsbuild, @apfeiffer1, @Degano, @nclopezo can you please review it and eventually sign? Thanks.
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.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #3948 was updated. @apfeiffer1, @Dr15Jones, @vlimant, @davidlange6, @monttj, @cmsbuild, @franzoni, @thspeer, @StoyanStoynev, @slava77, @ggovi, @vadler, @Degano, @ktf, @nclopezo can you please check and sign again.

David Abdurachmanov added 2 commits May 22, 2014 11:29
LLVM/Clang on AArch64 will report that most AST nodes are not
definition, which is not true. The following modifies script to
check definition based on location information directly (file, line,
 and column).

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

davidlt commented May 22, 2014

Updated (missed two is_definition() calls) and rebased. Tessted on CMSSW_7_2_X_2014-05-22-0200 in the following way:

rm -rf src
cp -L -r $CMSSW_RELEASE_BASE/src .
scram b -v -j 12 precompile 2>&1 | tee b.log
grep condformats_serialization_generate.py b.log | cut -d' ' -f3 | sort | xargs -n1 md5sum > current_md5

scram b vclean # Deletes all files on ./tmp
cp -L -r $CMSSW_RELEASE_BASE/src .
# Modifications on top
scram b -v -j 12 precompile 2>&1 | tee b2.log
grep condformats_serialization_generate.py b2.log | cut -d' ' -f3 | sort | xargs -n1 md5sum > new_md5

diff new_md5 current_md5

No differences reported.

@cmsbuild
Copy link
Contributor

Pull request #3948 was updated. @ggovi, @cmsbuild, @apfeiffer1, @Degano, @nclopezo can you please check and sign again.

@apfeiffer1
Copy link
Contributor

+1
provided Jenkins agrees ...

On Thu, May 22, 2014 at 11:32 AM, cmsbuild notifications@github.com wrote:

Pull request #3948 #3948 was
updated. @ggovi https://github.com/ggovi, @cmsbuildhttps://github.com/cmsbuild,
@apfeiffer1 https://github.com/apfeiffer1, @deganohttps://github.com/degano,
@nclopezo https://github.com/nclopezo can you please check and sign
again.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3948#issuecomment-43866596
.

Thanks,
cheers, andreas

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes or unless it breaks tests.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

ktf added a commit that referenced this pull request May 22, 2014
ARM64 -- condformats_serialization_generate.py: support for aarch64
@ktf ktf merged commit b1b3f86 into cms-sw:CMSSW_7_2_X May 22, 2014
@nclopezo nclopezo mentioned this pull request May 23, 2014
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

4 participants