Closed Bug 979913 Opened 6 years ago Closed 6 years ago

Build warnings in mozglue/

Categories

(Core :: mozglue, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Details

Attachments

(3 files, 1 obsolete file)

There are some annoying warnings when building mozglue on Gonk.
Comment on attachment 8386159 [details] [diff] [review]
[02] Bug 979913: Conditionally define PAGE_SIZE

Review of attachment 8386159 [details] [diff] [review]:
-----------------------------------------------------------------

I would rather not take this.  This code is all extremely specific to b2g, which should only ever have 4 KB pages.
Attachment #8386159 - Flags: review?(khuey) → review-
I don't think your assumption is correct. ARM also supports 1KiB and 64KiB AFAIK, x86 also supports 4MiB pages. Given the large amount of memory on even low-end hardware and the resulting overhead from page tables, it's likely that page sizes larger than 4KiB will be used at some point.

Anyway, this updated patch still fixes the compiler warning about redefining PAGE_SIZE, but keeps the page size hard-coded to 4KiB and emits a warning if the assumption of 4KiB page sizes is not true.

Changes to v1:

  - keep PAGE_SIZE at 4KiB
  - emit warning if the target might use a different page size
Attachment #8386159 - Attachment is obsolete: true
Attachment #8387467 - Flags: review?(khuey)
Where are we using PAGE_SIZE directly? AFAIK that's heavily frowned upon, we should be using getpagesize() or sysconf(_SC_PAGESIZE) if the former is unavailable. Maybe we should go ahead and fix that instead of defining PAGE_SIZE.
(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Where are we using PAGE_SIZE directly? AFAIK that's heavily frowned upon, we
> should be using getpagesize() or sysconf(_SC_PAGESIZE) if the former is
> unavailable. Maybe we should go ahead and fix that instead of defining
> PAGE_SIZE.

Good point. I opened bug 982569 for this. I'd still like to get the patch here merged, because it fixes an annoying build warning.
(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Where are we using PAGE_SIZE directly? AFAIK that's heavily frowned upon, we
> should be using getpagesize() or sysconf(_SC_PAGESIZE) if the former is
> unavailable. Maybe we should go ahead and fix that instead of defining
> PAGE_SIZE.

The usual reason is that PAGE_SIZE is then a compile-time constant, which the compiler can use to optimize multiplications, divisions, etc.  Also don't have to worry about initializing your kPageSize variable and so forth.
(In reply to Nathan Froyd (:froydnj) from comment #8)
> (In reply to Gabriele Svelto [:gsvelto] from comment #6)
> > Where are we using PAGE_SIZE directly? AFAIK that's heavily frowned upon, we
> > should be using getpagesize() or sysconf(_SC_PAGESIZE) if the former is
> > unavailable. Maybe we should go ahead and fix that instead of defining
> > PAGE_SIZE.
> 
> The usual reason is that PAGE_SIZE is then a compile-time constant, which
> the compiler can use to optimize multiplications, divisions, etc.  Also
> don't have to worry about initializing your kPageSize variable and so forth.

Sure, and optimizing such operations away are a valid goal. But the code in the file of patch [02] won't work reliably if your page differs from the hard-coded constants.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> Sure, and optimizing such operations away are a valid goal. But the code in
> the file of patch [02] won't work reliably if your page differs from the
> hard-coded constants.

I think that's Kyle's point in comment 4.
Comment on attachment 8386159 [details] [diff] [review]
[02] Bug 979913: Conditionally define PAGE_SIZE

Ok, that's a decent argument.
Attachment #8386159 - Attachment is obsolete: false
Attachment #8386159 - Flags: review- → review+
backed out the checkins from this bug in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=aa60e2265dde as requested from Thomas since the wrong version from one patch was checked in (via irc)
You need to log in before you can comment on or make changes to this bug.