Page MenuHomePhabricator

[XRay][AArch64] An attempt to fix test patching-unpatching.cc by flushing the instruction cache after code modification
ClosedPublic

Authored by rSerge on Dec 20 2016, 11:57 AM.

Diff Detail

Event Timeline

rSerge updated this revision to Diff 82129.Dec 20 2016, 11:57 AM
rSerge retitled this revision from to [XRay][AArch64] An attempt to fix test patching-unpatching.cc by flushing the instruction cache after code modification.
rSerge updated this object.
rSerge added reviewers: dberris, rengolin.
rSerge added subscribers: iid_iunknown, llvm-commits.
rengolin accepted this revision.Dec 22 2016, 9:54 AM
rengolin edited edge metadata.

LGTM. Thanks!

This revision is now accepted and ready to land.Dec 22 2016, 9:54 AM
rSerge closed this revision.Dec 22 2016, 10:59 AM

This broke the build. Did you manage to build this on AArch64? How did you test this?

@rSerge You forgot to add a forward declaration for __clear_cache to make it visible like e.g. trampoline_setup.c does.

This broke the build. Did you manage to build this on AArch64? How did you test this?

I cross-compiled compiler-rt project from x86_64-Windows to AArch64-Linux and then tested on QEmu-AArch64-Linux . I thought that the built-in doesn't need any header file inclusion. Apparently for different host platforms the built-in behaves differently, perhaps that's due to compiler version. I'll look into this.

Right, maybe the forward declaration as Oleg said would work.

rSerge added a comment.EditedDec 23 2016, 6:22 AM

As discussed here , presence or absence of __clear_cache built-in depends on the compiler options. The built-in is common to gcc and clang, just currently in clang it is implemented via a called function in compiler-rt, but (another version of) gcc may inline it as an intrinsic. As discussed here , earlier there was a bug in gcc that gcc was expecting the parameters to be void*, in contradiction to the documentation which says they are char*, and clang developers implemented this bug too (so that parameters are void*), and I understood that clang developers were then fixing this bug.

However, in compiler-rt, file clear_cache.c the function is still defined as void __clear_cache(void *start, void *end) { ... }, and it seems used in file trampoline_setup.c via a forward declaration also as extern void __clear_cache(void* start, void* end);.

So it seems that the best I can do to let the code compile with different gcc and clang versions is to forward-declare the function as extern void __clear_cache(void* start, void* end); and keep the reinterpret_cast<char*> so that the code accepts both the function and the intrinsic (but this may lead to ambiguity for some compiler version and flags).

rSerge reopened this revision.Dec 23 2016, 10:04 AM
This revision is now accepted and ready to land.Dec 23 2016, 10:04 AM
rSerge updated this revision to Diff 82415.Dec 23 2016, 10:05 AM
rSerge edited edge metadata.

Added the forward declaration for ____clear_cache() to fix the build.

This should work. Let's have another try.

rSerge closed this revision.Dec 23 2016, 1:31 PM

The second iteration has reached mainline in revision 290452 .

Still undefined symbol:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-39vma/builds/2086/steps/ninja%20check%201/logs/FAIL%3A%20XRay-aarch64-linux%3A%3Afixedsize-logging.cc

undefined reference to `__clear_cache(void*, void*)'

Reverted in r290453.

Next step, we'll test native first before commit.

Happy holidays!

I have been able to reproduce the problem when cross-compiling from x86_64-Linux to AArch64-Linux. Investigating now...

rSerge updated this revision to Diff 82928.EditedJan 3 2017, 12:13 PM

At a minimum, during x86_64-Linux to AArch64-Linux cross-compilation it didn't compile because of extern "C" missing. And the compiled program runs well on QEmu-AArch64-Linux .
@rengolin , what do you mean by "testing native" ? If that's compiling LLVM on AArch64 machine (i.e. without cross-compilation), then it would be challenging for me to do because on QEmu a LLVM build takes about a week, and the OS in the Arm32/AArch64 machines our team has is not exactly Linux.

rSerge reopened this revision.Jan 10 2017, 3:59 AM

Ping?

This revision is now accepted and ready to land.Jan 10 2017, 3:59 AM
rovka added a subscriber: rovka.Jan 10 2017, 4:05 AM

Hi,

Looks like this slipped through the cracks, I'll do a native run for you today and let you know how it goes.

Thanks.

Hi,

Looks like this slipped through the cracks, I'll do a native run for you today and let you know how it goes.

Thanks.

Thanks!

rovka accepted this revision.Jan 10 2017, 6:53 AM
rovka added a reviewer: rovka.

Hi,

Looks good on our test AArch64 machine, feel free to commit.

Diana

rSerge closed this revision.Jan 10 2017, 8:27 AM

Reached mainline in revision 291568.