Add cross platform wide string formatting specifier for NSPR logging

RESOLVED FIXED

Status

defect
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: bbondy, Assigned: wtc)

Tracking

Firefox Tracking Flags

(firefox11- affected, firefox12- affected, firefox13- affected)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

8 years ago
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
Reporter

Comment 1

8 years ago
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.
Reporter

Comment 2

8 years ago
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.
Assignee

Comment 3

8 years ago
bbondy: thank you for the info.  Let's support just %ls.
Reporter

Comment 4

8 years ago
ok sounds good.
Reporter

Comment 5

8 years ago
Attachment #573393 - Flags: review?(ted.mielczarek)
Reporter

Comment 6

8 years ago
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.
Attachment #573395 - Flags: review?(robert.bugzilla)
Comment on attachment 573395 [details]
Replaced %S with %ls for maintenance service code

looks good!
Attachment #573395 - Flags: review?(robert.bugzilla) → review+
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?
Attachment #573393 - Flags: review?(ted.mielczarek) → review+
Wan-Teh, did you want to give this a once-over?
Reporter

Comment 10

8 years ago
> 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? :)
Assignee

Comment 11

8 years ago
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 '('.
Reporter

Comment 12

8 years ago
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.
Attachment #573395 - Attachment is obsolete: true
Attachment #573395 - Attachment is patch: false
Reporter

Comment 13

8 years ago
Posted image 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.
Reporter

Comment 14

8 years ago
Fixed as per the last review comments, please advise if any further changes are needed.
Attachment #573393 - Attachment is obsolete: true
Attachment #573699 - Flags: review+
(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.
Reporter

Comment 16

8 years ago
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.
Assignee

Comment 17

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

Comment 18

8 years ago
:wtc NP I won't change.

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

Updated

8 years ago
Blocks: 481815
No longer depends on: 481815
Reporter

Updated

8 years ago
No longer blocks: 481815
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).

Updated

8 years ago
Blocks: 713936

Updated

8 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Reporter

Comment 21

8 years ago
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.
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
Reporter

Comment 23

8 years ago
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.
(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
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?
Reporter

Comment 26

8 years ago
> 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.
Reporter

Updated

8 years ago
Attachment #573699 - Flags: review+
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 ?
Reporter

Comment 28

8 years ago
Why is there a rush to move this patch up?
Reporter

Comment 29

8 years ago
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.
(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.
(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.
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
Reporter

Comment 33

8 years ago
> 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.
Assignee

Comment 34

8 years ago
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.
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.

Updated

8 years ago
Blocks: 727167
No longer blocking NSPR 4.9, as it has been released.
Removing dependency.
No longer blocks: 713936
Assignee

Updated

7 years ago
No longer blocks: 727167
Assignee

Comment 37

7 years ago
The compiler warnings are originally reported in
https://codereview.appspot.com/6197060/
Attachment #621819 - Flags: review?(netzen)
Reporter

Updated

7 years ago
Attachment #621819 - Flags: review?(netzen) → review+
Assignee

Comment 38

7 years ago
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
Assignee

Updated

7 years ago
Attachment #621819 - Flags: checked-in+
Reporter

Updated

6 years ago
Assignee: netzen → wtc
Reporter

Updated

4 years ago
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.