Open Bug 1307958 Opened 8 years ago Updated 2 years ago

-Werror=sign-compare smorgasbord

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect

Tracking

()

People

(Reporter: ishikawa, Assigned: ishikawa)

Details

Attachments

(5 files, 9 obsolete files)

Fix for -Werror=sign-compare in ./intl subdirectory.
11.05 KB, patch
Details | Diff | Splinter Review
Fix for -Werror=sign-compare in /nsprpub subdirectory.
20.81 KB, patch
Details | Diff | Splinter Review
Fix for -Werror=sign-compare in ./media subdirectory.
45.94 KB, patch
Details | Diff | Splinter Review
Fix for -Werror=sign-compare in ./js subdirectory.
3.63 KB, patch
Details | Diff | Splinter Review
Fix for -Werror=sign-compare in ./security subdirectory.
83.96 KB, patch
Details | Diff | Splinter Review 
On my local linux build, I applied -Werror=sign-compare all over the tree (C-C, M-C) and the attached patches were necessary to get the software to build.
I have omitted a few features from since I was compiling C-C, and so there may be more offenders, but this is a start.

Someone might want to polish this up.

I know some of the issues are in third-party libraries and so they may needed to be sent to upstream.

Anyway, this is a start.

I collect the patches per top-level sub-directory of M-C.
This is for ./intl subdirectory patch.
I know this is a third party library, correct?
Assignee: nobody → ishikawa
This fix is in /media subdirectory.
Fix in /js subdirectory.
These patches were created in haste to make C-C TB compile when -Werror=sign-compare is applied all over the tree (but some subdirectories may be exempted because I did not enable all the features for C-C TB build in my MOZCONFIG file).

Anyway, this is a quick and dirty fix to get past -Werror=sign-compare and someone might find useful for his/her local build needs.

I don't claim all the fixes are quite correct although I don't see egregious errors in try-comm-central submissions.

TIA
Fixing bitrot in recent m-c tree.
Attachment #8798187 - Attachment is obsolete: true
Fixing bitrot.
Attachment #8798189 - Attachment is obsolete: true
Fixing bitrot.
Attachment #8798188 - Attachment is obsolete: true
Updating this patch together with other patches.
Attachment #8798190 - Attachment is obsolete: true
This is new.
Since late November and early December last year, 
I found I need to add patches to the compilation of files under ./security subdirectory if I add -Werror=sign-compare directive to my CFLAGS and CXXFLAGS.
Comment on attachment 8826657 [details] [diff] [review]
Fix for -Werror=sign-compare in ./security subdirectory.

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

If you put this up on phabricator, I'll review it.  We are using https://nss-review.dev.mozaws.net/ where you can request an account.
(In reply to Martin Thomson [:mt:] from comment #11)
> Comment on attachment 8826657 [details] [diff] [review]
> Fix for -Werror=sign-compare in ./security subdirectory.
> 
> Review of attachment 8826657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you put this up on phabricator, I'll review it.  We are using
> https://nss-review.dev.mozaws.net/ where you can request an account.

Please advise me of
 - how to obtain an account there, and
 - what is the workflow (I mean "put this up" is not entirely clear to me, even. Please recall I am a volunteer, a user of TB who submit patches now and then when bugs interfere with TB operation including performance issues).

Do I need a special tool (aside from mercurial, git, etc.)?

TIA!
Forgot to place a request for information.
Flags: needinfo?(martin.thomson)
If you try to create an account, a request will be sent to the administrators.  You don't need to be a Mozilla employee.  We have folks at Red Hat already using the system.  If you have trouble, or the adminstrator asks someone to confirm, cc me (or point them at this comment).

The best way to do this is use the arcanist tool: https://secure.phabricator.com/book/phabricator/article/arcanist/  This is the tool that Firefox as a whole is moving to, so you should have a head start on that process.

One you have that installed, `arc set-config default "https://nss-review.dev.mozaws.net/"` then `arc install-certificate` should get you started.

I usually create a commit, then submit that with `arc diff --create .^`, though `arc diff` should be enough.
Flags: needinfo?(martin.thomson)
(In reply to Martin Thomson [:mt:] from comment #14)
> If you try to create an account, a request will be sent to the
> administrators.  You don't need to be a Mozilla employee.  We have folks at
> Red Hat already using the system.  If you have trouble, or the adminstrator
> asks someone to confirm, cc me (or point them at this comment).
> 
> The best way to do this is use the arcanist tool:
> https://secure.phabricator.com/book/phabricator/article/arcanist/  This is
> the tool that Firefox as a whole is moving to, so you should have a head
> start on that process.
> 
> One you have that installed, `arc set-config default
> "https://nss-review.dev.mozaws.net/"` then `arc install-certificate` should
> get you started.
> 

Thank you for the tips. I installed aracnist commands and am now waiting for the
account to be approved.
Once it is approved, I think I will do
arc install-certificate since I need an API token from the login page: whatever that means :-)

> I usually create a commit, then submit that with `arc diff --create .^`,
> though `arc diff` should be enough.

So I have to create a local commit, don't I?
I have been using mqueue extension to hg (mercurial) command, and so am not sure
what I could do with the patch queues instead of creating a local commit. But I will see what I can do.

TIA
hg mqueue creates commits, so it should work fine.
(In reply to Martin Thomson [:mt:] from comment #16)
> hg mqueue creates commits, so it should work fine.

Nice to hear that. So many tools to submit patches and get reviewed :-)
I am waiting for the account creation approval now.

TIA
(In reply to Martin Thomson [:mt:] from comment #14)
> If you try to create an account, a request will be sent to the
> administrators.  You don't need to be a Mozilla employee.  We have folks at
> Red Hat already using the system.  If you have trouble, or the adminstrator
> asks someone to confirm, cc me (or point them at this comment).
> 
> The best way to do this is use the arcanist tool:
> https://secure.phabricator.com/book/phabricator/article/arcanist/  This is
> the tool that Firefox as a whole is moving to, so you should have a head
> start on that process.
> 
> One you have that installed, `arc set-config default
> "https://nss-review.dev.mozaws.net/"` then `arc install-certificate` should
> get you started.
> 
> I usually create a commit, then submit that with `arc diff --create .^`,
> though `arc diff` should be enough.

Hi, I have verified my e-mail address to the confirmation e-mail when I tried to create the account.
But when I access https://nss-review.dev.mozaws.net/, I get the following message.
Wait for Approval
Your account has been created, but needs to be approved by an administrator. You'll receive an email once your account is approved. [wait patiently]

I wonder if you can get in contact with someone at the above and see what is going.

TIA
I've pinged the administrator.
(In reply to Martin Thomson [:mt:] from comment #19)
> I've pinged the administrator.

Thank you! I have received a notification of account creation finally!
I will figure out how to upload the latest patches on my local PC over the weekend.

One less queue in my local patch set is a good thing!

Thank you again.
Reminder to upload when you get a chance.
Flags: needinfo?(ishikawa)
Yes, will do as soon as I sorted out local build bustage issue. (I cleaned up the patch against the latest source tree when I hit
Bug 1369324 - Thunderbird treeherder build busted as of 2017-06-01 - Running setup.py bdist_wheel for blessings: finished with status 'error' - ImportError: No module named six )
I also wanted to clean up the patch issues regarding 
Bug 1242030 - Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500
but that can wait.
fixed bitrot.
Attachment #8826648 - Attachment is obsolete: true
fixed bitrot.
Attachment #8826650 - Attachment is obsolete: true
fixed bitrot
Attachment #8826652 - Attachment is obsolete: true
fixed bitrot
Attachment #8826655 - Attachment is obsolete: true
fixed bitrot
Attachment #8826657 - Attachment is obsolete: true
(In reply to Martin Thomson [:mt:] from comment #21)
> Reminder to upload when you get a chance.

Dear Martin,
I have created 
https://nss-review.dev.mozaws.net/T1
I am not sure if "T1" is the right moniker or not.
I was not even sure how to proceed from there, then
it gradually dawned on me.
Should I pull (from where?) the /nss/ directory using |git| command (everybody seems to use |git| instead of |hg| when it comes to phabricator), and then create a local commit of my changes and then
submit the change for review (to where)?.

Phabricator seems to be very handy tool, but there are things an unfamiliar 
passerby needs to understand
 - phabricator itself. (I am trying to learn),
 - particular setting of phabractor for this nss-review.dev.mozaws.net such as where the source git repo lives and where
   the review ought to go [this is probably self-evident once the location of source repo is known.]


BTW, those submitting patches to nss-review.dev.mozaws.net ought to set -Werror in their testing
to make all warnings behave as error.
This is the sure way to force developers to conform to strict C/C++ interpretation.
Yes, third-party software makes this difficult to carry out.

TIA
T1 is definitely not right.  We don't use Maniphest on phabricator. 

Here is what you should do:

a) Check out NSS.  Mercurial or git are both fine; use what ever you prefer.

b) Apply your patch to that tree (use patch -p3 to strip security/nss) and create a commit.

c) Run arc diff - this should create a new "Differential" with a name like "D3XX".  Post the link here.

The NSS build runs with -Werror enabled; you should add -Wsign-compare to several places in the build configuration to get it to trigger:

1. coreconf/werror.py
2. coreconf/Werror.mk
3. remove -w44018 from coreconf/WIN32.mk
4. remove -w44018 from coreconf/config.gypi

See https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Building for building instructions.  Building NSS on its own is a lot faster than building Firefox.
Flags: needinfo?(ishikawa)
Oh, I forgot one thing.  If you have tryserver access, you can test NSS by pushing to our tryserver.  See instructions here: https://wiki.mozilla.org/NSS:TryServer  These builds almost never fail, so you don't have to wade through tons of test failures to work out if your change was sane.
Comment on attachment 8873511 [details] [diff] [review]
Fix for -Werror=sign-compare in /nsprpub subdirectory.

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

::: nsprpub/lib/libc/src/plgetopt.c
@@ +148,5 @@
>      if (internal->minus == 2) 
>      {
>          char * foundEqual = strchr(internal->xargv,'=');
> +        PRIntn optNameLen = foundEqual ? (int) (foundEqual - internal->xargv) :
> +                            (int) strlen(internal->xargv);

I think that if you are going to use size_t (as below), then you should make optNameLen size_t straight out.

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Assignee: nobody → ishikawa
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: