589744 - js/src/perf/pm_linux.cpp doesn't build on some architectures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Fallback to perf measurement stub when perf_event_open syscall isn't defined

For some reason, __NR_perf_event_open is not defined on debian unstable ia64 (which could very well be a bug, but still...). In this case falling back to the stub should be in order.

Comment 1

[Mike Hommey [:glandium]]

It doesn't build on alpha either.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 468263 [details] [diff] [review]
Fallback to perf measurement stub when perf_event_open syscall isn't defined

I kind of wonder whether this wouldn't be better done in configure.in, but I'm hardly the world's foremost build-system aestheticist to say it would be, and therefore should be.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 468263 [details] [diff] [review]
Fallback to perf measurement stub when perf_event_open syscall isn't defined

> I kind of wonder whether this wouldn't be better done in configure.in, but I'm
> hardly the world's foremost build-system aestheticist to say it would be, and
> therefore should be.

It could be done in configure.in, but that sounds like extra cruft for something that really is a corner case (__NR_perf_event_open should really be defined on these platforms). Let's see what bsmedmerg has to say about it.

Comment 4

[Mike Shaver (:shaver emeritus)]

We should put this in configure, because that's where crufty details about platform detection should go.

But if this is really just a bug in debian unstable IA64, then I think debian should just fix that bug, and until they they won't be able to use trunk Firefox on unstable IA64.  Don't see that we should carry any complexity to cover that narrow intersection, tbh.  (If for some reason we do cruft up our codebase to work around this bug, we should reference the specific debian bug report in a comment in configure, so that we can check back and see when debian has fixed and we can remove it.)

But if it's to work around a bug in how debian builds its IA64

Comment 5

[Mike Hommey [:glandium]]

It might actually not be debian related, but be a linux kernel bug. The definition is apparently in asm-generic/unistd.h.

Comment 6

[Mike Shaver (:shaver emeritus)]

Well, let's find out before we add noise to the JS engine.  Can you report back here when it's sorted?  Thanks.

Comment 7

[Mike Hommey [:glandium]]

So, it turns out not all the supported linux architectures support the perf_event_open syscall, and that it's expected it's not defined on these.

I guess, then, a configure check would be better.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 468263 [details] [diff] [review]
Fallback to perf measurement stub when perf_event_open syscall isn't defined

Removing the r+ so that it doesn't show up in my stack of r+ed bugs, because bsmedberg did r-.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Fallback to perf measurement stub when perf_event_open syscall isn't supported

This checks whether the syscall is defined in the headers on top of checking for the perf_event.h header.

Comment 10

[jag (Peter Annema)]

Comment on attachment 513405 [details] [diff] [review]
Fallback to perf measurement stub when perf_event_open syscall isn't supported

s/_even_/_event_/g

"sizeof(__NR_perf_event_open)" should give you the same result with less code, and allows __NR_perf_event_open to be e.g. an int instead of a #define (I doubt this is really an issue)

Comment 11

[Mike Hommey [:glandium]]

Attachment: Fallback to perf measurement stub when perf_event_open syscall isn't supported

Addressed jag's comments. What are the current rules for the tracemonkey branch, to get this landed eventually?

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 519880 [details] [diff] [review]
Fallback to perf measurement stub when perf_event_open syscall isn't supported

AIUI, tracemonkey is open for regular checkins currently.

Comment 13

[Mike Hommey [:glandium]]

http://hg.mozilla.org/tracemonkey/rev/82129f60f6f7

Comment 14

[Mike Hommey [:glandium]]

Attachment: Fallback to perf measurement stub when perf_event_open syscall isn't supported, as landed

It turns out the second patch was actually an interdiff, so this combines both, and is what landed.

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/82129f60f6f7