Closed Bug 589744 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: glandium, Assigned: glandium)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

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.
Attachment #468263 - Attachment is patch: true
Attachment #468263 - Attachment mime type: application/octet-stream → text/plain
Attachment #468263 - Flags: review?(jwalden+bmo)
Assignee: general → mh+mozilla
It doesn't build on alpha either.
Summary: js/src/perf/pm_linux.cpp doesn't build on ia64 → js/src/perf/pm_linux.cpp doesn't build on some architectures
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.
Attachment #468263 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #468263 - Flags: review?(benjamin)
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
It might actually not be debian related, but be a linux kernel bug. The definition is apparently in asm-generic/unistd.h.
Well, let's find out before we add noise to the JS engine.  Can you report back here when it's sorted?  Thanks.
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.
Attachment #468263 - Flags: review?(benjamin) → review-
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-.
Attachment #468263 - Flags: review+
This checks whether the syscall is defined in the headers on top of checking for the perf_event.h header.
Attachment #468263 - Attachment is obsolete: true
Attachment #513405 - Flags: review?(khuey)
Attachment #513405 - Flags: review?(khuey) → review?(ted.mielczarek)
Attachment #513405 - Flags: review?(ted.mielczarek) → review+
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)
Addressed jag's comments. What are the current rules for the tracemonkey branch, to get this landed eventually?
Attachment #513405 - Attachment is obsolete: true
Attachment #519880 - Flags: review?(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.
Attachment #519880 - Flags: review?(ted.mielczarek) → review+
It turns out the second patch was actually an interdiff, so this combines both, and is what landed.
Attachment #519880 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/82129f60f6f7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.