This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][OMPT] Fix locking testcases for 32 bit architectures
ClosedPublic

Authored by protze.joachim on Feb 20 2019, 8:30 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

protze.joachim created this revision.Feb 20 2019, 8:30 AM
hbae accepted this revision.Feb 20 2019, 1:11 PM

LGTM

This revision is now accepted and ready to land.Feb 20 2019, 1:11 PM
hans added a subscriber: hans.Feb 21 2019, 12:38 AM
hans added inline comments.
runtime/test/ompt/synchronization/lock.c
13 ↗(On Diff #187591)

I don't know this code at all obviously, but will this work with the PRIu64 conversion specifier? If ompt_wait_id_t is 32-bit on 32-bit platforms, this sounds like it wouldn't match?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 12:50 AM
protze.joachim marked an inline comment as done.Feb 21 2019, 4:02 AM
protze.joachim added inline comments.
runtime/test/ompt/synchronization/lock.c
13 ↗(On Diff #187591)

ompt_wait_id_t is defined to be 64 bit by the OpenMP spec.

The problem is the expansion to 64 bit in the cast.
gcc and clang seem to have a different idea of what should be the result on 32 bit arch for

`int a;
(uint64_t)&a;`

I still can reproduce the issue when using gcc as test compiler after compiling the runtime with clang or vice versa.

mgorny added inline comments.Feb 21 2019, 5:22 AM
openmp/trunk/runtime/test/CMakeLists.txt
34

Is this really supposed to be part of this fix? NetBSD buildbot got broken between r354552..r354559 (http://lab.llvm.org:8011/builders/lldb-amd64-ninja-netbsd8/builds/19082) and this looks like the only change in openmp in that batch.

This should be fixed in rOMP354572. As a bot owner, could you please make sure that the bot lists the commits to openmp? Otherwise it's hard to detect broken bots without emails.

This should be fixed in rOMP354572. As a bot owner, could you please make sure that the bot lists the commits to openmp? Otherwise it's hard to detect broken bots without emails.

If I only knew how, I'd have fixed that long ago.

This should be fixed in rOMP354572. As a bot owner, could you please make sure that the bot lists the commits to openmp? Otherwise it's hard to detect broken bots without emails.

If I only knew how, I'd have fixed that long ago.

If I had to bet, that's because lldb-amd64-ninja-netbsd8 declares itself to be an lldb builder: https://github.com/llvm/llvm-zorg/blob/master/buildbot/osuosl/master/config/builders.py#L976
As a result it will only be triggered by commits to llvm, cfe and lldb: https://github.com/llvm/llvm-zorg/blob/master/buildbot/osuosl/master/master.cfg#L194
And if buildbot doesn't know that the bot magically updates and tests openmp, it probably doesn't add the relevant commits and so on.

protze.joachim marked an inline comment as done.Feb 21 2019, 6:44 AM
protze.joachim added inline comments.
runtime/test/ompt/synchronization/lock.c
13 ↗(On Diff #187591)

https://reviews.llvm.org/D58506 casts the addresses to uintptr_t before expanding to 64 bit.