979913 - Build warnings in mozglue/

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

There are some annoying warnings when building mozglue on Gonk.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 979913: Fix GCC warning about undeclared function |snprintf|

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 979913: Conditionally define PAGE_SIZE

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 979913: Remove unused variables

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 979913: Conditionally define PAGE_SIZE (v2)

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

Comment 6

[Gabriele Svelto [:gsvelto]]

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.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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.

Comment 8

[Nathan Froyd [:froydnj]]

(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.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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.

Comment 10

[Nathan Froyd [:froydnj]]

(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 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8386159 [details] [diff] [review]
[02] Bug 979913: Conditionally define PAGE_SIZE

Ok, that's a decent argument.

Comment 12

[Carsten Book [:Tomcat]]

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)

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks a lot, Kyle!

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5e13d2c003
https://hg.mozilla.org/integration/mozilla-inbound/rev/6659e34090c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e673315c346f

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e673315c346f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a5e13d2c003
https://hg.mozilla.org/mozilla-central/rev/6659e34090c9
https://hg.mozilla.org/mozilla-central/rev/e673315c346f