699567 - Add cross platform wide string formatting specifier for NSPR logging

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Brian R. Bondy [:bbondy]]

In Bug 481815 patch 8, we added support for a %S formatting specifier for wide string output in NSPR logging.

This bug is to:
1) Add %ls in the same way doing the same thing.
2) Add support for all platforms not just Windows

Comment 1

[Brian R. Bondy [:bbondy]]

Just an FYI from bug 481815:

> Emanuel Hoogeveen 2011-11-03 16:16:17 PDT
> For the record, MinGW-w32/w64 4.7.0 reject %S when compiling with -pedantic. 
> %ls appears to be ISO-C++.

I don't think there's any harm in accepting both of them though.

Comment 2

[Brian R. Bondy [:bbondy]]

Just an FYI in case anyone is interested in the history of %S vs. %ls format specifiers.

The C++ 2003 standard doesn't make any mention of it that I can find.
The C99 standard does though:

"The original intent was to add the new conversion specifiers %S and %C to the existing formatted input and output functions to handle a wide character string and a wide character respectively.  After long discussions about the actual implementation and future library directions, these specifiers were withdrawn. They were replaced with the qualified conversion specifiers, %ls and %lc, with the addition of %l[...] in the formatted input functions"

Likely some compilers implemented %S before it was standardized and never removed support.

Comment 3

[Wan-Teh Chang]

bbondy: thank you for the info.  Let's support just %ls.

Comment 4

[Brian R. Bondy [:bbondy]]

ok sounds good.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Cross platform NSPR logging with ls formatting string instead of S

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Replaced %S with %ls for maintenance service code

Should be a quick review, it will land at the same time as the other patch in this ticket.  Just replacing %S with the standard compliant %ls.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 573395 [details]
Replaced %S with %ls for maintenance service code

looks good!

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 573393 [details] [diff] [review]
Cross platform NSPR logging with ls formatting string instead of S

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

::: nsprpub/pr/src/io/prprf.c
@@ +588,5 @@
>  
>  	case 's':
> +	    if( nas[ cn ].type != TYPE_WSTRING) {
> +	      nas[ cn ].type = TYPE_STRING;
> +      }

The formatting here seems a little wonky. I guess the mix of tabs and spaces is throwing you off?

Comment 9

[(not currently active) Ted Mielczarek]

Wan-Teh, did you want to give this a once-over?

Comment 10

[Brian R. Bondy [:bbondy]]

> The formatting here seems a little wonky. 
> I guess the mix of tabs and spaces is throwing you off?

Yes, is there a method to the madness? :)

Comment 11

[Wan-Teh Chang]

Comment on attachment 573393 [details] [diff] [review]
Cross platform NSPR logging with ls formatting string instead of S

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

:bbondy: thanks for the patch.  The only problem is the indentation.
I explained how to use tabs in this file below.

::: nsprpub/pr/src/io/prprf.c
@@ +86,4 @@
>  	double d;
>  	const char *s;
>  	int *ip;
> +	const wchar_t *ws;

If ordering doesn't matter, can you list 'ws' right below 's'
because they're related?

Or do you want to keep them in the same order as the corresponding
TYPE_xxx macro definitions below.  That seems like a good idea.

@@ +588,5 @@
>  
>  	case 's':
> +	    if( nas[ cn ].type != TYPE_WSTRING) {
> +	      nas[ cn ].type = TYPE_STRING;
> +      }

In this file, set tab stop to 8.  This file seems to use
tabs, so follow that style.

Add a space after "if" but remove the space after the opening
parenthesis '('.

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 573395 [details]
Replaced %S with %ls for maintenance service code

This maintenance service patch is already r+ but it will be collapsed into the consolidated patch.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Whitespace montage

The file using a really good mix of i) spaces only, ii) tabs only, and iii) tabs and spaces intermixed.
I did find one inconsistency with the closing tab on that block with surrounding code though, so I fixed that.
Please see the screenshots showing a montage of some of the formatting inconsistencies.

Each part of the file is a bit different but I tried to stay consistent with the parts of the file for whitespace.

It probably wouldn't be a bad idea to globally unify to either tabs or whitespace in a separate patch.   This would lose hg blame / annotate info though.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Cross platform NSPR logging with ls formatting string instead of S. v2.

Fixed as per the last review comments, please advise if any further changes are needed.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Brian R. Bondy [:bbondy] from comment #13)
> It probably wouldn't be a bad idea to globally unify to either tabs or
> whitespace in a separate patch.   This would lose hg blame / annotate info
> though.

Whitespace cleanups just introduce an additional step in a process that usually involves multiple steps anyways, so it doesn't really significantly impact the usefulness of hg annotate (and certainly doesn't "lose" it). You shouldn't let blame concerns stop you from cleaning up whitespace.

Comment 16

[Brian R. Bondy [:bbondy]]

OK thanks Gavin.  wtc or ted please confirm that I should cleanup whitespace to 8 spaces and no tabs, or if you would prefer the normal Mozilla 2 spaces, or if you don't want it at all.  

I'll post and do the cleanup in the context of another bug/patch since it isn't related to this task.

Comment 17

[Wan-Teh Chang]

:bbondy: don't fix formatting problems in code that you aren't
modifying.  Thanks.  I will take care of fixing the whitespace
problem.

Comment 18

[Brian R. Bondy [:bbondy]]

:wtc NP I won't change.

Is the patch good to go?  Whitespace is consistent with surrounding code.

Comment 19

[Kai Engert [:KaiE:]]

Sigh.

We need an NSPR 4.9 release for mozilla-beta, which currently uses NSPR 4.9 beta.

The mozilla-beta branch is supposed to be restricted to the smallest amount of possible changes.

If you insist on "support %ls, only",
the consequence is that we must change mozilla-beta code, in addition to the NSPR change.

Can we be totally sure that we fully understand the amount of code (outside of NSPR) that would have to be adjusted? If not, we shouldn't do it. Searching through all 1119 occurrences of %S in mozilla would be too much work.

My personal opinion is, there should be only two scenarios that are acceptable at this time of the release schedule.


Scenario (1)
Under the assumption that missing cross platform for %S has zero potential for harm (such as crashes):
- support only %ls
- make the change to NSPR
- outside of mozilla-beta, change only the pieces added by bug 481815

If you want to go for this scenario, we must involve mozilla-beta drivers QUICKLY (TODAY) to make them aware of this need, and get their approval.


Or, if scenario (1) has risk for harm, or if drivers refuse approval, then I think we should do:


Scenario (2)
- accept the reality that it's too late for widespread changes in mozilla-beta
- support both %S and %ls
- adjust the NSPR patch accordingly.


My personal vote is (2).

Comment 20

[Kai Engert [:KaiE:]]

akeybl, see my comment 19

Comment 21

[Brian R. Bondy [:bbondy]]

All compilers that I know of support both %S and %ls so I would prefer that as well.  This is still waiting on Comment 18 though.   The beta should use the as is library with %S.

Comment 22

[Kai Engert [:KaiE:]]

I have a scenario 3:

If you're saying, the existing code, which only supports Windows, is totally fine for Firefox 11, Firefox 12 and Firefox 13, because the logging for Windows is sufficient, and because the existing code has zero risk of causing on other platforms,

then I'd say:

Scenario (3)
- Don't change anything now
- accept that this is a Windows only support at this time
- get things fixed in NSPR 4.9.1

Comment 23

[Brian R. Bondy [:bbondy]]

This is not a regression that needs to be fixed. On Windows you have the ability to do wide string formatting via %S with NSPR logging. We should also support %ls moving forward, and on all platforms in the future which is what this patch is for. 

The original %S was added to simplify a lot of code in the maintenance service which is windows only code, but we moved away from NSPR logging in the end, so unless someone else used it, it is unused.

Comment 24

[Kai Engert [:KaiE:]]

(In reply to Brian R. Bondy [:bbondy] from comment #23)
> 
> we moved away from NSPR logging in
> the end, so unless someone else used it, it is unused.

But it's unknown if anyone else started to use it, correct?
So that might be a risk.

On the other hand, if support for %S was windows-only code, the people who introduced theoritical other changes using %S might have noticed? Or maybe not because they developed on Windows only?


You said, you moved away from using this code at all.
Does this mean the following scenario 4 is valid?


Scenario (4)
- revert the portions of the changes from bug 481815 that added
  windows-only support for %S to NSS
- release this is NSPR 4.9 final
- get everything else sorted later

Comment 25

[Kai Engert [:KaiE:]]

My new preference is scenario 3.

It means that NSPR 4.9 would contain support for %S, on windows only.
Because of this, we'll have to guarantee that %S continues to work in the future.

Can you confirm that shipping NSPR 4.9 as is has zero risk for crashes or misbehaviour on the other platforms, assuming nobody has started to use %S?

Comment 26

[Brian R. Bondy [:bbondy]]

> But it's unknown if anyone else started to use it, correct?
> So that might be a risk.

It is possible that Windows only code used %S so I think it should not be removed at this point.  

As is, there is no backwards regression risk because there is no regression. 
If you remove %S then there is a regression risk for anyone that tried to use it. 

I'm fine with reverting the other task if you want and if you are sure no one else used it, but I think this is good support to have in NSPR logging and simplifies code.

> Can you confirm that shipping NSPR 4.9 as is has zero risk for crashes or misbehaviour 
> on the other platforms, assuming nobody has started to use %S?

I don't know how anyone could guarantee that. 
I do know that I suspect it has no misbehavior and I suspect it has no risk of crashing.

Again, I don't see this as effecting any old code because I don't see this as a regression. 
I see it as an existing enhancement that can stand on its own and a new enhancement that makes it cross platform and adds a specifier.
I do think that enough time has passed that we should not remove %S support though.

Comment 27

[Kai Engert [:KaiE:]]

After hearing Brian's arguments, in my opinion we should do scenario 2.

How quickly can we get a new patch for NSPR, that adds cross platform support for both %S and %ls ?

Comment 28

[Brian R. Bondy [:bbondy]]

Why is there a rush to move this patch up?

Comment 29

[Brian R. Bondy [:bbondy]]

It's my understanding that the %S code is already tested in production but that the %ls cross platform code in this patch is not tested in production.  So I don't understand why we'd want to take on any risk of regressions from this patch.

Comment 30

[Kai Engert [:KaiE:]]

(In reply to Brian R. Bondy [:bbondy] from comment #28)
> Why is there a rush to move this patch up?

Because mozilla-beta uses NSPR 4.9 BETA

Because mozilla-beta will change to FF 11 soon…

Because by policy, it is forbidden to ship NSPR BETA snapshots in 
official Mozilla releases.

This means, we must produce a release of NSPR 4.9 final immediately.

The only question is, what is the snapshot that should be released with least risks and with least amount of required changes.

Comment 31

[Kai Engert [:KaiE:]]

(In reply to Brian R. Bondy [:bbondy] from comment #29)
> It's my understanding that the %S code is already tested in production but
> that the %ls cross platform code in this patch is not tested in production. 
> So I don't understand why we'd want to take on any risk of regressions from
> this patch.

But the %S code is only tested on Windows, correct?

If there is zero risk keeping things this way, shipping only %S for Windows, zero risk for non-windows platforms, I'm personally fine with this way.

This would be scenario 3.

Comment 32

[Kai Engert [:KaiE:]]

akeybl, FYI:

Bug 713936 implicitly includes permission to update NSPR, too.

(Because NSS and NSPR releases always go together, and both snapshots
 in mozilla-beta/aurora are currently NSPR beta/NSS beta,
 and must be updated to final releases for official Mozilla releases
 by policy.)

We must figure out the safest path for releasing NSPR 4.9

Comment 33

[Brian R. Bondy [:bbondy]]

> But the %S code is only tested on Windows, correct?

It is only meant to be used on Windows but I tried to make sure there was no problems on Ubuntu and Windows at the time of writing the patch.

There are ifdefs for Windows around all the code here:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=481815&attachment=569786
Which was included in these pushes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/765f5b79fedf
https://hg.mozilla.org/mozilla-central/rev/765f5b79fedf

> If there is zero risk keeping things this way, shipping only %S for Windows, 
> zero risk for non-windows platforms, I'm personally fine with this way.

I can't say there is 0 risk as mentioned earlier. 
I don't believe that any code uses it in FF11 though. 
Even on today's mozilla-central a search for %S results in no matches for NSPR logging.

I think it is safest to just not include the patch from bug 481815 and not rush a second patch which is more dangerous than the one in bug 481815.

Comment 34

[Wan-Teh Chang]

Attachment: Subset of bbondy's attachment 573699 to change %S to %ls (for reference only)

Thank you for your comments.

By inspecting all the checkins in bug 481815, I verified what
Mozilla's updater code is not using PR_*printf functions for
logging.  Ultimately UpdateLog::Printf calls the standard C
library function vfprintf:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/common/updatelogging.cpp#85

Since the support for the %S format specifier is not advertised,
I don't think anyone else is using it.  Moreover, anyone using a
feature in a Beta release and should be prepared for such minor
changes.

I have already extracted a safe subset of bbondy's patch in this
bug that merely change %S to %ls, without changing anything else.
But I see that my proposal to change %S to %ls has generated a
flurry of comments.  It is now a lot of work to explain why I
think it is safe.  After all, it was my fault to forget to check
in bbondy's patch in November.  I should respect the judgment of
Ted "luser" and accept the consequences of my inaction.

Comment 35

[Kai Engert [:KaiE:]]

I've just learned that Firefox 10 shipped with NSPR 4.9 beta.
That's bad!

I would like to very stronly request that NSPR 4.9 ships with at least the same functionality as beta.

If NSPR 4.9 beta supports %S, then 4.9 release must support 4.9, too.

Comment 36

[Kai Engert [:KaiE:]]

No longer blocking NSPR 4.9, as it has been released.
Removing dependency.

Comment 37

[Wan-Teh Chang]

Attachment: Fix compiler warnings

The compiler warnings are originally reported in
https://codereview.appspot.com/6197060/

Comment 38

[Wan-Teh Chang]

Comment on attachment 621819 [details] [diff] [review]
Fix compiler warnings

Patch checked in on the NSPR trunk (NSPR 4.9.1).

Checking in prprf.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prprf.c,v  <--  prprf.c
new revision: 3.23; previous revision: 3.22
done