Closed Bug 730805 Opened 8 years ago Closed 6 years ago

MFBT should provide format macros from inttypes.h

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: froydnj, Assigned: Waldo)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Doing this would help avoid compiler format string warnings, at least (people assuming that PRInt64/int64_t should be printed with %lld, for instance).
I've had a partial patch for this floating in a tree for awhile but haven't found the time to finish it off.  Soon, I hope.
Assignee: nobody → jwalden+bmo
Same song and dance as for stdint.h, now for the inttypes.h header.  This adds the MSVC emulation, next patch will actually hook it up.
Attachment #668749 - Flags: review?(gerv)
...and the second part still needs some troubleshooting, so it's not getting posted tonight.  :-)  Maybe Sunday if I have time to debug things.
Not done yet, but close.  Ish.

https://tbpl.mozilla.org/?tree=Try&rev=b01a36b5abcc

Not sure what's up with Armv7a there, since mozilla-config.h is in the command line for those.  The Windows cases look like they might be bugs in the MSIntTypes.h header in the previous patch, under certain build configurations; particularly at least int_fast16_t seems to have the wrong SCN{i,d}16 specifiers, as if that type were int16_t rather than int32_t as it actually is.  The OS X failure I'm hoping is just infra fail, but who knows.

Android's perhaps oddest, where the NDK <inttypes.h> support seems to have a ridiculous number of format mismatches.  Dunno what we'll want to do about that, if it's not just user error on my part.

The test is kind of an epic one.  But it (plus compiler warnings) should smoke out any inconsistencies we have here right quick.  I hope.  It worked for the Windows results in the try push, at least...

fprintf and fscanf are not very good interfaces.
Comment on attachment 668749 [details] [diff] [review]
1 - Import msinttypes's inttypes.h implementation for MSVC

r+, but this requires a standard patch to about:license if this code ships with Firefox (which I assume it does). Probably a section titled "MSIntTypes.h License".

Gerv
Attachment #668749 - Flags: review?(gerv) → review+
Depends on: 812218
I didn't know it at the start of this bug, but apparently the scanf functions are really pretty dangerous, because an out-of-target-range number induces undefined behavior.  (Which makes me look somewhat askance at the 300-odd scanf hits in our tree -- granted some are to NSPR's reimplementation which might not have this problem, but I kind of doubt it.)  That MSVC can't actually implement the SCN* macros fully (it lacks specifier support to scan into a char) seals the deal.

New plan, then: provide a header only guaranteed to implement the PRI* portions of the <inttypes.h> interface.

The mozilla-config.h.in changes, to have effect, must apply before any inclusion of <stdint.h>.  This effectively requires mozilla-config.h always be the first include.  It's not on b2g, which adds a gonk-misc/Unicode.h include before we get to mozilla-config.h, and Unicode.h drags in <stdint.h>.  Moving CFLAGS/CXXFLAGS to the end of the commandline, as bug 812218 will do, allows this patch to work.

https://tbpl.mozilla.org/?tree=Try&rev=44de530f5fe1

It is perhaps worth noting that it looks like some systems' <stdint.h> and <inttypes.h> are inconsistent in how they define the fixed-size types and the corresponding PRI* macros, triggering format warnings.  I'm not sure what can or should be done about this; ideas appreciated.
Attachment #668757 - Attachment is obsolete: true
Attachment #682053 - Flags: review?(respindola)
I am not too familiar with our use of NSPR. Do we always use the NSPR implementations? If so, why is it a problem that windowns doesn't support reading into a 8 bit position? Does the NSPR implementation support it?

What is the intended use of MOZ_CUSTOM_INTTYPES_H? Doing

#  include MOZ_CUSTOM_INTTYPES_H

would break trying to use distcc-pump, which worked some time in the past :-(

Which systems have inconsistent stdint.h and inttypes.h? Can we pretend they don't have inttypes.h and use our implementation like on windows?
These macros are for use with libc's printf/, not with whatever NSPR provides as a printf replacement.  NSPR doesn't come into play here at all -- if it has macros for use with PRUint8 and similar, those are its problem.

We currently provide MOZ_CUSTOM_STDINT_H as a sop to SpiderMonkey embedders, on Windows.  Some SpiderMonkey embedders also embed other projects, and those other projects might have implemented their own emulation of the <stdint.h> types.  For example, see bug 479258.  As an escape hatch in such places, we let embedders provide their own custom <stdint.h> implementation.  As long as they make it compatible with any other project's <stdint.h> types, they can compile.

Since <inttypes.h> must be compatible with <stdint.h> (actually it has to #include it), if <stdint.h> is reimplemented, it's also necessary for <inttypes.h> to be reimplemented compatible with the <stdint.h> reimplementation.  That's the purpose served by MOZ_CUSTOM_INTTYPES_H.  If it breaks distcc-pump, it's not obvious to me why it'd break it any more than MOZ_CUSTOM_STDINT_H already breaks it.  I've heard no reports of MOZ_CUSTOM_STDINT_H breaking anyone, let alone it breaking distcc-pump.

It's pretty likely that once we've bumped compilation requirements to MSVC10, we'll eliminate MOZ_CUSTOM_STDINT_H and just always use <stdint.h>.  (This would let us get rid of MOZ_CUSTOM_INTTYPES_H as well, likely.  There's no intrinsic value to enabling overriding -- we just do it because we have to.)  In the meantime, however, this rather-ugly hackaround was enough to get us using the standard typedefs, so it was worth it.  No released version of MSVC supports <inttypes.h> yet, however, so it'll be awhile before this new header would go away.

I'm not sure which systems have inconsistent headers, except that I saw some in tinderbox logs when pushing this to try.  I think it may have been Android that had issues; I'm not certain offhand.  We could use our own implementations in such cases, although it'd probably be messy to detect, and to get it all working just so wrt include-guards and such.
> Since <inttypes.h> must be compatible with <stdint.h> (actually it has to
> #include it), if <stdint.h> is reimplemented, it's also necessary for
> <inttypes.h> to be reimplemented compatible with the <stdint.h>
> reimplementation.  That's the purpose served by MOZ_CUSTOM_INTTYPES_H.  If
> it breaks distcc-pump, it's not obvious to me why it'd break it any more
> than MOZ_CUSTOM_STDINT_H already breaks it.  I've heard no reports of
> MOZ_CUSTOM_STDINT_H breaking anyone, let alone it breaking distcc-pump.

It uses an include scanner that would fail to find what this include points to. Given that this would just be a problem on windows if not using our headers, that is probably fine.

> It's pretty likely that once we've bumped compilation requirements to
> MSVC10, we'll eliminate MOZ_CUSTOM_STDINT_H and just always use <stdint.h>. 
> (This would let us get rid of MOZ_CUSTOM_INTTYPES_H as well, likely. 
> There's no intrinsic value to enabling overriding -- we just do it because
> we have to.)  In the meantime, however, this rather-ugly hackaround was
> enough to get us using the standard typedefs, so it was worth it.  No
> released version of MSVC supports <inttypes.h> yet, however, so it'll be
> awhile before this new header would go away.

OK

> I'm not sure which systems have inconsistent headers, except that I saw some
> in tinderbox logs when pushing this to try.  I think it may have been
> Android that had issues; I'm not certain offhand.  We could use our own
> implementations in such cases, although it'd probably be messy to detect,
> and to get it all working just so wrt include-guards and such.

Agreed. I guess it can also be done as a followup patch.

I guess the only other comment I have is to ask if it would be possible to define  __STDC_CONSTANT_MACROS and friends on the command line. Looks like this would depend on bug 812218, is that OK?

I ask because the same header behaving differently in different translation units can causes some fairly annoying bugs to hunt down. The current proposal for adding modules to c++ also ignores macros defined in files, but uses the ones defined in the command line, so this would be more (distant) future proof.
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #9)
> I guess it can also be done as a followup patch.

Yeah, that's what I was thinking of doing.

> I guess the only other comment I have is to ask if it would be possible to
> define  __STDC_CONSTANT_MACROS and friends on the command line. Looks like
> this would depend on bug 812218, is that OK?

We effectively define them on the commandline now, we just shove them in a header that gets included through the commandline.  I'm not sure why we put them in a file rather than as -D entries on the commandline -- some sort of personal/reviewer preference, I guess.  Interestingly we're not 100% consistent in this, as we have a -DNO_NSPR_10_SUPPORT that's added to the commandline now.
Attachment #682053 - Flags: review?(respindola) → review+
Blocks: 818689
Try:

https://hg.mozilla.org/try/rev/3fa894362774

Push:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8b33922178
https://hg.mozilla.org/integration/mozilla-inbound/rev/a90c62555235

If something odd happens, back me out, I'm only fighting insomnia right now and won't do so myself.  :-)
https://hg.mozilla.org/mozilla-central/rev/7d8b33922178
https://hg.mozilla.org/mozilla-central/rev/a90c62555235
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 920108
Depends on: 945029
Duplicate of this bug: 911542
Depends on: 1118529
You need to log in before you can comment on or make changes to this bug.