839852 - ASan builds fail with "unknown type name 'size_t'" in Asan.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Simon Montagu :smontagu]

The checkin for bug 723228 broke ASan builds:

In file included from /home/smontagu/mozwork/hgtree/mozilla/js/src/ds/LifoAlloc.cpp:8:
In file included from /home/smontagu/mozwork/hgtree/mozilla/js/src/ds/LifoAlloc.h:11:
./../../dist/include/mozilla/ASan.h:16:63: error: unknown type name 'size_t'
  void __asan_poison_memory_region(void const volatile *addr, size_t size)
                                                              ^
./../../dist/include/mozilla/ASan.h:18:65: error: unknown type name 'size_t'
  void __asan_unpoison_memory_region(void const volatile *addr, size_t size)

The obvious fix is to back out the reordering of the #includes in http://hg.mozilla.org/mozilla-central/diff/d49cff40b5c3/js/src/ds/LifoAlloc.h, but it might be more bullet-proof just to #include stddef.h in ASan.h

Comment 1

[Razvan Cojocaru]

> h, but it might be more bullet-proof just to #include stddef.h in ASan.h

Is the <stddef.h>-style #include the preferred way, or should that be #include <cstddef>?

Comment 2

[Razvan Cojocaru]

Attachment: Included stddef.h in ASan.h

Stddef.h it is.

Comment 3

[Simon Montagu :smontagu]

Comment on attachment 712243 [details] [diff] [review]
Included stddef.h in ASan.h

Looks good to me, but I'm not a peer for this code.

Comment 4

[Simon Montagu :smontagu]

(though I can testify that the patch compiles)

Comment 5

[Razvan Cojocaru]

I see, thanks for changing the reviewer!

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 712243 [details] [diff] [review]
Included stddef.h in ASan.h

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

You want <stddef.h> because <cstddef> isn't guaranteed to define ::size_t.  It might provide it, or it might not -- only guarantee is it'd provide std::size_t.

Comment 7

[Jeff Walden [:Waldo]]

...and it looks like someone already got there first:

changeset:   121464:df066a85126f
user:        Christian Holler <[e-mail]>
date:        Sun Feb 10 13:11:04 2013 +0100
summary:     No bug - Include missing header in mfbt/ASan.h. r=me

Bleeding edge can be fun sometimes, eh?

Comment 8

[Razvan Cojocaru]

> You want <stddef.h> because <cstddef> isn't guaranteed to define ::size_t. 
> It might provide it, or it might not -- only guarantee is it'd provide
> std::size_t.

Quite, however I usually prefer to use the C++ headers and either use std::size_t, or add "using std::size_t;". I don't see having to use std::size_t as a problem.

Comment 9

[Jeff Walden [:Waldo]]

Perhaps the perfect image of a modern major C++ codebase (sorry, couldn't resist :-) ) would use std::size_t everywhere.  Mozilla, however, is a C++ codebase with strong C influences from having grown up in a time when C++ compiler support was (and to some extent remains) distinctly uneven, so we're more likely to use C headers than the corresponding C++ headers, if we're not using functionality only in the C++ headers.

There are also times where we actively care about our code being the C subset of C++.  This header isn't such a time yet, but I could see it becoming one at some point.  (Tho I'd rather we stopped using C and just used C++ exclusively [even if it's only the C subset of C++], but that's a harder battle to fight.)

Comment 10

[Razvan Cojocaru]

No problem. Thank you for explaining.