Fixed bitrot by accommodating the clang-format changes of ldap directory.

One more patch which is posted in tandem is necessary to shut up format issues, etc. completely.

There are a few issues after the type changes, etc. in the format string.
This patch has to be applied in tandem to shut up format string issues found by GCC.

Both of these patches need to be applied AFTER the patch in
Bug 1277602
LDAP: Debug print macro LDAPDebug passes incorrect # of args to format strings and produced many WARNINGs at compile time.

Bitrot - Updated to the clang-formatted source tree.

Fixt bitrot due to clang-format change.

Just to check - this LDAP C-SDK in C-C is the canonical version, right? There's no upstream we should be tracking or anything?
(Looking at the headers, it looks like it's from the Netscape days, but LDAP is corporate enough that I could imagine one of the bigcorps picking it up and running with it :- )

Yeah, no upstream.

Yes, this was an external library but it seems we do not plan to upgrade it (if there is no upstream) and do not maintain it much.
But if you want to fix the compiler warnings and can do so without introducing new bugs, we can surely accept the patch. Thanks.

(In reply to :aceman from comment #14)

Comment on attachment 9090155 [details] [diff] [review]
fix clang-format BITROT: patch for printf format related issues in ldap
source tree.

Review of attachment 9090155 [details] [diff] [review]:

::: ldap/c-sdk/libraries/libldap/tmplout.c
@@ +617,5 @@

       }
       if (html) {
         sprintf(buf, "<DD>%s<BR>%s", p, eol);
       } else {

•        snprintf( buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol );
  

Why are these leading spaces added in some cases?

This could be tab vs untabify (tabs converted to space issues.)
I hope my running the code via local formatter to recreate the patch would help.

(In reply to :aceman from comment #15)

Yes, this was an external library but it seems we do not plan to upgrade it (if there is no upstream) and do not maintain it much.
But if you want to fix the compiler warnings and can do so without introducing new bugs, we can surely accept the patch. Thanks.

As far as my testing goes locally and on tryserver, we don't see additional errors. (I wonder how much testing LDAP gets, though.)

Anyway, as I mentioned elsewhere, this set of patches are applied in my tryserver runs for sometime and I don't see any errors from the patch set.
See, for example, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=545674bbbce29f7c6e7b92b63e277a76ac5ceb68
The patches have been thrown in to my tryserver runs since mid July or sometime around it. (I had to recreate patch to apply to the latest clang-formatted C-C tree anyway.)

(In reply to :aceman from comment #16)

Comment on attachment 9090154 [details] [diff] [review]
clang-format change bitrot: ldap-sign-compaldap-sign-compare.patch (with
ber_len_t change, too)

Review of attachment 9090154 [details] [diff] [review]:

::: ldap/c-sdk/include/ldap-extension.h
@@ +811,5 @@

#define LDAP_UTF8COPY(d, s)
((0x80 & *(unsigned char )(s)) ? ldap_utf8copy(d, s) : (((d) = (s)), 1))
+
+#define LDAP_UTF8GETCC(s) ((0x80 & (unsigned char)(s)) ? ldap_utf8getcc (&s) : (unsigned long) s++)
+#define LDAP_UTF8GETC(s) ((0x80 & (unsigned char)(s)) ? ldap_utf8getcc ((const char)&s) : (unsigned long) *s++)

This is getting a bit long and will probably not pass by our code
reformatter.

So I think I would rather let the formatter handle the accepted line break, etc.
I don't have any preference here EXCEPT that I kept the original line fomratting as much as possible and
try to mimic the original line formatting if I needed to create a new line similar to the existing ones.

So my idea is to get the review for logical changes, and then modified the code accordingly LOGICALLY speaking, and then
run the formatter locally to produce a new patch to conform to the formatter's idea of proper formatting.

What do you think?
If I run the formatter before the full review, we may lose a bit of ease of comparison against the old copy.
But given the tool for comparing ("diff") on bugzilla, I have to agree this is moot.

::: ldap/c-sdk/libraries/libldap/getoption.c
@@ +415,5 @@

   aip->ldapai_vendor_name = NULL;
   return (LDAP_NO_MEMORY);
 }

• for (i = 0; (unsigned) i < NSLDAPI_EXTENSIONS_COUNT; ++i) {

At some for()s you seem to add some more indent (maybe tabs?). Please check
those.

So I can run cflow-format locally and produce a new set of patches without touch the code at all otherise.

What is preferable?

(In reply to ISHIKAWA, Chiaki from comment #17)

•        snprintf( buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol );
  

Why are these leading spaces added in some cases?
This could be tab vs untabify (tabs converted to space issues.)
I hope my running the code via local formatter to recreate the patch would help.

I mean the space before "buf". Surely it is not any formatter adding those?

I'm not sure we indend to run the C++ clang formatter on the ldap folder anytime soon (maybe also because this seems to be pure C?) so I think we should do any line wraps manually in this patch.

The leading spaces before for() are here:
--- a/ldap/c-sdk/libraries/libprldap/ldappr-io.c
+++ b/ldap/c-sdk/libraries/libprldap/ldappr-io.c
@@ -261,33 +261,33 @@ prldap_poll(LDAP_X_PollFD fds[], int nfd

•  for (j = 0; j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
  

•        for (j = 0; (unsigned) j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
  

We can surely drop these from the patch manually, no need to run any reformatter and get many more lines touched.

Formatting has happened:
https://hg.mozilla.org/comm-central/rev/0c50a354ca18
Reformat to Google coding style in ldap/. rs=reformat
https://hg.mozilla.org/comm-central/rev/96a805927f55
replace tabs with spaces in ldap/ C files and fix comments badly formatted by reformatting. rs=white-space-only
https://hg.mozilla.org/comm-central/rev/dc3b0d8e7139
Repeat running clang-format on ldap/ and mailnews/mime. rs=reformat

(In reply to :aceman from comment #20)

(In reply to ISHIKAWA, Chiaki from comment #17)

•        snprintf( buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol );
  

Why are these leading spaces added in some cases?
This could be tab vs untabify (tabs converted to space issues.)
I hope my running the code via local formatter to recreate the patch would help.

I mean the space before "buf". Surely it is not any formatter adding those?

Oh, I think that is me :-)

(In reply to :aceman from comment #21)

I'm not sure we indend to run the C++ clang formatter on the ldap folder anytime soon (maybe also because this seems to be pure C?) so I think we should do any line wraps manually in this patch.

The leading spaces before for() are here:
--- a/ldap/c-sdk/libraries/libprldap/ldappr-io.c
+++ b/ldap/c-sdk/libraries/libprldap/ldappr-io.c
@@ -261,33 +261,33 @@ prldap_poll(LDAP_X_PollFD fds[], int nfd

•  for (j = 0; j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
  

•        for (j = 0; (unsigned) j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
  

We can surely drop these from the patch manually, no need to run any reformatter and get many more lines touched.

I think we can run formatter now as Jorg pointed out in comment 22.
I would prefer to have the formatter to fix my whitespace faux pa if possible.
But there may be fine print.

(In reply to Jorg K (GMT+2) from comment #22)

Formatting has happened:
https://hg.mozilla.org/comm-central/rev/0c50a354ca18
Reformat to Google coding style in ldap/. rs=reformat
https://hg.mozilla.org/comm-central/rev/96a805927f55
replace tabs with spaces in ldap/ C files and fix comments badly formatted by reformatting. rs=white-space-only
https://hg.mozilla.org/comm-central/rev/dc3b0d8e7139
Repeat running clang-format on ldap/ and mailnews/mime. rs=reformat

Jorg, one thing I need to know.
There seems to be some type of configuration file to control the fomrating.
The necessary file for formating ldap directory (and the subdirectories below) is in the hg repository and
so in my local copy, too, correct?

OR is there a fine print warning?
As you mentioned in your comment above, you took care of whitespace issues separately.

replace tabs with spaces in ldap/ C files and fix comments badly formatted by reformatting. rs=white-space-only
DO WE HAVE TO HANDLE TAB and SPACE specially by hand?!
That is if I run cflow-format again, it will mess up your tabs vs. whitespaces change?

If the above is cleared up, I take that based on Jorg's post to Maildev mailing list on April 23,
Subject: [Maildev] Reformatting the comm-central C++ code base to Google coding style

If you want to try it out: mach clang-format -p comm/dir/of/your/choice, issued from the M-C top source directory.

I can run the above command for
the following directories AFTER I apply each patch, and obtain the new patch that contains the formatting approved by the official formatting
including space/tab issues.
(And repeat this for each patch.)

comm/ldap/c-sdk/include
comm/ldap/c-sdk/libraries/libprldap
comm/ldap/c-sdk/libraries/liblber
comm/ldap/c-sdk/libraries/libldif
comm/ldap/c-sdk/libraries/libldap

Correct?

I was a bit hesitant to do this yet since I still use "hg MQ" extension to create patches.
This MQ usage is now officially deprecated or abandoned style of hg usage for source management.
I realized this after the following problem: the venerable |mach bootstrap| command, if I am not carefull enough,
created .hg file that breaks MQ usage.
A couple of months or so ago, for experiment, I turned on automatic formatting in checking when |mach bootstrap|
suggested to turn on this feature. It touches and modifies .hg file.
Unfortunately, this feature messed up MQ patches.
(For whatever the reason, the automatic editing feature for message (-m) of "hg MQ" qnew or qrefr started to be invoked TWICE after I enabled automatic formatting, and by the time the second editor invocation happened, the original patch content was erased or something...
So I disabled the automatic formatting function in check-in, etc. for now.

Of course, I can manually run the above hopefully without messing up the source tree, I hope.
As long as the necessary configuration file is there, I should be OK even for tabs vs whitespaces issue.

No?

TIA

I forgot to mention.
For the second part of the patch set,

fix clang-format BITROT: patch for printf format related issues in ldap source tree.

I throw in the use of snprintf instead of the original sprintf since there were some dubious printing that may really goes close to the string size limit.
However, generally speaking, I think the use of original sprintf was rather safe, but why NOT accept the warning and modify the code to use safer coding practice?
There were cases when I manually guessed the size of a scalar value and number of digits or characters necessary for printing, and if my guessed range was correct, the use of sprintf without bound checking seemed OK. But such checking should be left to the computer.

The file that configures the formatting is in the tree, .clang-format, at the top level.

You can run mach clang-format -p comm/ldap/c-sdk/include, or in subdirectories then refresh your patch.

clang-format sadly don't replace tabs, but unless I missed something, there shouldn't be any tabs anywhere in the source tree any more.

mach bootstrap may refresh your Mercurial version. I use it from time to time, I also use HG queues, and I haven't seen any problem.

I hope I've answered everything. Please ask precise questions, that's easier then to scan a large block of text for questions.

Dear Jorg,

Thank you for the answers.

(In reply to Jorg K (GMT+2) from comment #25)

The file that configures the formatting is in the tree, .clang-format, at the top level.

You can run mach clang-format -p comm/ldap/c-sdk/include, or in subdirectories then refresh your patch.

I am a bit surprised that mach clang-format -p comm/ldap/c-sdk/include does not seem to touch the source tree on my PC at all.
after I applied my local patches.
I am afraid that |mach clang-format| is not working on my PC...
I don't think my hand-created patch conforms to c++ style completely.
(For example, I had a space " " after "(", but it did not complain. Maybe it is allowed by clang-format.)

clang-format sadly don't replace tabs, but unless I missed something, there shouldn't be any tabs anywhere in the source tree any more.

I will check my patch to see if I introduced any tabs or something.
There is one instance where the whitespace indentation at the beginning of a line is messed up.

mach bootstrap may refresh your Mercurial version. I use it from time to time, I also use HG queues, and I haven't seen any problem.

|mach bootstrap| is OK as long as you DON'T ENABLE automatic clang-formatting at HG commit (even locally).
If you do, somehow the editor that you have specified for editing the message (-m) for qrefr or qnew gets invoked TWICE IN SUCCESSION and
the second time, the editor is invoked your patch becomes an empty patch :-(

I hope I've answered everything. Please ask precise questions, that's easier then to scan a large block of text for questions.

Thank you.

I am afaaid that somehow |mach clang-format| is not working on my PC.
I disabled the "automatic" formatting on check-in feature suggested by |mach bootstrap| manually by commenting out a couple of lines from .hgrc. That may have something to do the apparent non-invocation.
OR my hand-crafted code conforms to google style completely(?!).
Even the whitespace issues mentioned by aceman in comment 21 is not touched by |mach clang-format|? (Again this is unlikely so that makes me think that |mach clang-format| is a noop on my PC at this moment.)

I tried to see if enabling the clang-format, js-format extensions in .hgrc would enable |clang-format| to correctly.
No I don't think so.
And trying that I think I messed my local source tree. Definitely enabling the formatting on commit (locally) even in the form of
hg MQ is fatal.
I will recreate my source tree over the weekend.

I have no idea how to go about this |mach clang-format| issue.
Is it possible that someone can apply the patches and then re-post the formatted patch on bugzilla?

TIA

If it's not working, you probably have some odd modification in some configuration. Do these, and then see if things start working, and make sure you use the correct paths

hg qpop --all
hg purge
hg pull -u && hg up default
cd ..
./mach bootstrap

(In reply to Magnus Melin [:mkmelin] from comment #28)

If it's not working, you probably have some odd modification in some configuration. Do these, and then see if things start working, and make sure you use the correct paths

hg qpop --all
hg purge
hg pull -u && hg up default
cd ..
./mach bootstrap

Thanks, I will report my findings over the weekend.

(In reply to ISHIKAWA, Chiaki from comment #29)

(In reply to Magnus Melin [:mkmelin] from comment #28)

If it's not working, you probably have some odd modification in some configuration. Do these, and then see if things start working, and make sure you use the correct paths

hg qpop --all
hg purge
hg pull -u && hg up default
cd ..
./mach bootstrap

Thanks, I will report my findings over the weekend.

I am sorry that the above did not quite worked well.

Do people use the hg's feature of enabling us to edit, using an editor, the message when we do NOT specify "-m messagestring" to the following commands?
hg qnew
hg qrefersh

I do and hg invokes a specified editor (in my case, it is emacs usually, but I tested the operation with vi as well).
When I enable clang-format on check-in, the editor is invoked TWICE and trashed the patch.

HOWEVER, I think if I specify "-m messagestring" to the above commands, and do NOT invoke the editor as a result, I do not trash patch at all.
This I recall with 20/20 hindsight.

BTW, will some kind soul send me the working ~/.hgrc config file with which you are certain that |mach clang-format ...| works?
I have manually edited .hgrc (and obviously disabled a couple of lines related to clang-format upon checkin due to the problems I described.).
Comparision with working ~/.hgrc may help.

TIA

PS: In any case, I will create a small test directory under ./mozilla and
see if |mach clang-format ...| works on THAT subdirectory instead of "./comm".
I don't want to trash my real patches any more while I am testing the behavior of |mach clang-format ...|.

I have finally figured out at least the cause of the failure to run |mach clang-format -p pathname| successfully.

I have stored mozconfig under a non-default name of mozconfig-tb3 and set up various environment variables with which to run mozilla development tools.

I found out that I really needed to run my own script |./do-make-tb.sh clang-format -p pathname| to run |mach clang-format -p pathname| AFTER setting up these environment variables and such.
It would have been great if the bare |mach clang-format -p pathname| would complain about missing commands in the PATHs, or missing mozconfig, whatever, but it did not loudly and so I didn't realize this.

Now I have verified that clang-format did kick in to format my test source file under a test directory I created.

I still have no idea about why I expereince the multiple invocation of editor after enabling clang-format when one uses the editor to fill in the message for | hg qnew| or |hg qrefr|, but I have to be careful for now. | hg qnew -m message| or |hg qrefr -m message| does not invoke an editor and I don't trash patches even if I use MQ.

Once I run the LDAP patches through clang-format, I will re-upload the patches That should contain the correct format style after all and I hope the whitespace issues are taken care of automatically
.

TIA

Just to note, there is an issue with ./mach clang-format not picking up the formatting rules in comm/.clang-format under Linux (seems ok on Windows). Notes and workaround in Bug 1575449.

(In reply to Ben Campbell from comment #32)

Just to note, there is an issue with ./mach clang-format not picking up the formatting rules in comm/.clang-format under Linux (seems ok on Windows). Notes and workaround in Bug 1575449.

Thank you for heads-up. Yeah, I am using linux. I have to figure out where the tmp directory used by mach exactly.
I am specifying TMP, TMPDIR explicitly and so that may be where I should copy the .clang-format file.

Thank you again.

Now clang-formatted.
This should take care of my gratuitous handling of whitespace.

This builds locally and on tryserver.
See, e.g., https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8b1bbba78ca71568446d58bad2327ddb2dafda25

Now clang-formatted.
This should take care of my gratuitous handling of whitespace.

This builds locally and on tryserver.
See, e.g., https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8b1bbba78ca71568446d58bad2327ddb2dafda25

I am a bit put off by clang-format putting type cast without a space before an expression.
That is, I prefer
(int) x
rather than
(int)x
but maybe it is only me.

Aceman,

The patches have been clang-formatted.

From cursory reading, I believe all the whitespace issues that you noticed have been taken care of by clang-format.
Well, you and I may not some manners in which formatting happens, but that is life :-)

Can you kindly take a look at this?

(In reply to :aceman from comment #38)

Comment on attachment 9100332 [details] [diff] [review]
clang-formatted: patch for printf format related issues in ldap source tree.

Review of attachment 9100332 [details] [diff] [review]:

::: ldap/c-sdk/libraries/liblber/io.c
@@ +613,5 @@

char msg[80];
ber_err_print("*** sos dump ***\n");
while (sos != NULLSEQORSET) {

• sprintf(msg, "ber_sos_dump: clen %lu first 0x%p ptr 0x%p\n",

•        (long unsigned int)sos->sos_clen, sos->sos_first, sos->sos_ptr);
  

This sos_clen is ber_len_t and that one is 'unsigned int' or 'size_t' (in
your patch). So why cast it to long unsigned int? In the next file you cast
it to 'long long int'.

@@ +814,5 @@

#ifdef LDAP_DEBUG
if (lber_debug) {
char msg[80];

• sprintf(msg, "ber_get_next: tag 0x%x len %lu contents:\n", ber->ber_tag,

•        (long unsigned int)ber->ber_len);
  

I think this one is ber_len_t too.

::: ldap/c-sdk/libraries/libldap/request.c
@@ +345,5 @@

         if (ext_data && ext_data->bv_len && ext_data->bv_val) {

•          LDAPDebug1(LDAP_DEBUG_TRACE,
  

•                     "nsldapi_send_server_request: Unsolicited response "
  

•                     "len: %lld\n",
  

•                     (long long int)ext_data->bv_len, 0, 0);
  

This bv_len is ber_len_t and that one is 'unsigned int' or 'size_t' (in your
patch). So why cast it to long long int? To get some common size that is
larger than ber_len_t on all platforms?

Some years ago, I found that compilers on different platforms used different sizes for some data types, and I could not
get simple SAME printf-like format string to behave sanely on all platforms.
(That was before I learned some [not complete] predefined strings for printing some scalar data types.
But even then, for maybe 10 days, I could not get Windows compiler on mozilla's compiler server farm to accept otherwise
good such strings. I think the compiler was broken in this regard for like two weeks during summer of 2017.)

So this is why I simply took it easy and extend it to be a possibly largest scalar type on 32-bit and 64-bit platform [and with
known compilers: clang, gcc, MS Visual Studio C compiler.].

A kludge, but it works :-(

Yes, I can understand that. Maybe that is why we often use the uint32_t and similar variables to force the same size on all platforms.
I can understand if you don't want to use those in this third party library that does not use them.

(In reply to :aceman from comment #41)

Comment on attachment 9100331 [details] [diff] [review]
clang-formatted: ldap-sign-compaldap-sign-compare.patch (with ber_len_t
change, too)

Review of attachment 9100331 [details] [diff] [review]:

::: ldap/c-sdk/libraries/libldap/getvalues.c
@@ +103,5 @@

} else {
rc = ber_scanf(&ber, "[v]", &vals);
}

• if (rc == (int)LBER_ERROR) {

rc is int, even though ber_scanf returns unsigned int (ber_tag_t).
But before this, they stuff strcasecmp (an int) into rc.

I wonder which way to go here.

Aceman, I am unavailable next week due to a big technological conference/exhibition
http://www.tronshow.org/index-e.html

so I will follow up on the issues the week after. Sorry about this.
I hope every is entering the festive season safe and sound.

Gosh, I must have been really exhausted, and failed to follow up on this.
Now is the time to fix this since Benc is working on Bug 1277602.

I am creating a set of patches that breaks down the big patch into smaller ones to explain the motivation and necessity of these patches.
That information would be useful to newcomers to this bugzilla.
I hope we can make an educated decision on problems such as the one in comment 41, i.e. the following with this background information.

rc is int, even though ber_scanf returns unsigned int (ber_tag_t).
But before this, they stuff strcasecmp (an int) into rc.

I wonder which way to go here.

I want to clear the local patch queue before tackling bug 1242030.
There is now only a single test failure when I submit tryserver job with the patch in bug 1242030 (and some).
That failure is from ldap-related test, and I wonder if that bug comes from the patch in this bugzilla.
That is why I want to have the patch landed and checked in beta or nightly.

Creating the divided patches to explain the motivation and necessity is a re-learning experience.
I hope once the patch set is done and uploaded here, we see clearly why and how the patch was created.

I have split the original patch into about a dozen patches as explained in my previous comment.
This ought to help me to figure out where a few strange LDAP test failure is introduced by which part of the patch.
It did help (!)
You can see a series of try-comm-central jobs where a first half the series of the patches don't introduce
LDAP-related errors, but later patches did.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6d00893de0f96493e6e7d796a0255516764d6781

After a bit more analysis, I am going to upload the set of split patches so that we can land the first half (safe) ones that do not introduce the ldap-test failures first.
There is also a potential (not sure) sprintf() buffer size issue and I replace it with snprintf().

It turns out that making the original ldap code 64-bit clean completely is a mess...
I am trying to figure out an easy way out.

The problem is with this line.
https://searchfox.org/comm-central/source/ldap/c-sdk/libraries/libldap/memcache.c#1451

    int scope = (int)pData2;

pData2 is a pointer to (void*).
We lose the upper 32-bit information here. (I think this was ONE of the reasons why I thought the code was not 64-bit clean.

Other causes were incorrect cast of the form in the following.
32_bit_scaler = (int) p1 - (int) p2
instead of
32_bit_scaler = (int) (p1 - p2)

Even if the subtraction, p1 - p2, produces 32-bit value, the original cast causes an issue, I think, when p1 and p2 are 64-bit address.

Anyway, it takes a lot longer than I thought to fix this 64-bit cleanness.

OTOH, I could finally run the build with my tentative patches WITHOUT causing ldap-related errors as in
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=96563a67f215e33cd770f5658ff9be4b3d00b3e5
and so maybe I am done with this bugzilla for now with those patches and aim for 64-bit cleanness in a different bugzilla.

What do people think?

Oops. Too early. There was one ldap error in mochitest (I think, though, it is caused by an invocation of I/O routine in non-threaded call or something to that effect.)

Back to square one...

(In reply to ISHIKAWA, Chiaki from comment #47)

OTOH, I could finally run the build with my tentative patches WITHOUT causing ldap-related errors as in
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=96563a67f215e33cd770f5658ff9be4b3d00b3e5
and so maybe I am done with this bugzilla for now with those patches and aim for 64-bit cleanness in a different bugzilla.

What do people think?

I think that's a good idea. Breaking up the work you're doing into smaller pieces makes it easier for the reviewer(s).

You might want to rebase again, I believe that some of those test failures have been fixed.

I am going to post a series of patches and explain why the patches were necessary and produced at each step somewhat.

Since a few small patches actually caused later build breakage, following patches take of the build break, and thus
each patch cannot be applied alone.
After the explanation of small patches, I will fold the failing patch and the rectifying patch to let build succeed at each step.
In the end, I hope either we come up with a large patch that consolidate the whole patches (with modifications based on feedback, of course),
or a series of patches that are applied in one go.

The try job looks promising. I don't see ldap-related error(s) in the following.
(I think.)
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=8cefc0b26170f9991e2f293633407a9e620aea46

The ldap-related error(s) showed up in my local testing as an I/O operation from non-main thread or something to that effect and not something my patches introduced as far as I could tell.

(In reply to :aceman from comment #41)

Comment on attachment 9100331 [details] [diff] [review]
clang-formatted: ldap-sign-compaldap-sign-compare.patch (with ber_len_t
change, too)

Review of attachment 9100331 [details] [diff] [review]:

::: ldap/c-sdk/libraries/libldap/getvalues.c
@@ +103,5 @@

} else {
rc = ber_scanf(&ber, "[v]", &vals);
}

• if (rc == (int)LBER_ERROR) {

rc is int, even though ber_scanf returns unsigned int (ber_tag_t).
But before this, they stuff strcasecmp (an int) into rc.

I wonder which way to go here.

In the later patch I am going to post, I decided to set ber_tag_t to "int" (!).
That was done quite recently (last week.)

But then , I have to change a function that returns the length of a string to return "int".
This is because such a function returns ((ber_tag_t) 0xFFFFFFFF), that is minus one, to be a value that signals it's error.

Oh well, the code sucks frankly.

I am going to upload the following patches that are the results of breaking the original patches into smaller chunks.
Some patches need to be combined for the patches to successfully build, but the upload is for initial review and discussion.
Consolidation can wait.

 3 U brand-new-1243121-ldap-part-0-format-string-and-type.patch: Bug 1243121 part-0: format string and snprintf buffer size issue
 4 U brand-new-1243121-ldap-part-1-simple.patch: Bug 1243121 part-1: eliminating signed vs unsigned warnings (now treated erro), first step
 5 U brand-new-1243121-ldap-part-2-address-minusone-comparison.patch: Bug 1243121 part-2: comparison of minus one vs pointer
 6 U brand-new-1243121-ldap-part-4-different-sign-extension-in-a-macro-local_-OK.patch: Bug 1243121 part-4 patch to fix different sign extension in a macro.
 7 U brand-new-1243121-ldap-part-5.5-ber_tag_t.patch: Bug 1243121 part-5.5 make ber_tag_t to int
 8 U brand-new-1243121-ldap-part-5.75-ber_tag_t.patch: Bug 1243121 part-5.75: LBER_DEFAULT signals an error of size_t function
 9 U brand-new-1243121-ldap-part-6-pointer-local-OK.patch: Bug 1243121 part-6: casting pointer difference to ber_len_t
10 U brand-new-1243121-ldap-part-6.5-memcache.patch: Bug 1243121 part-6: casting pointer difference to ber_len_t
11 U brand-new-1243121-ldap-part-8-os-ip.patch: Bug 1243121 part-8 patch to fix to eliminate warning for getsockopt().
12 U brand-new-1243121-ldap-part-10-tmplout.patch: Bug 1243121 part-10 patch annotate unused variable assignment for future review
13 U brand-new-1243121-ldap-part-20-memcache.patch: Bug 1243121 part-20 patch for memcache
14 U brand-new-1243121-ldap-part-30-decode.c.patch: Bug 1243121 part-30: decode.c to accommodte the new better type defintion
15 U brand-new-1243121-ldap-part-30.5-decode.c.fix.patch: Bug 1243121 part-30.5 patch to fix a couple additional bugs/warnings from part-30
16 U brand-new-1243121-ldap-part-40-fix-casts.patch: Bug 1243121 part-40: fix somewhat incorect casts (when ber_len_t was int)

The modern versions of GCC checks the size of buffer passed to sprintf() and format string issues.
This solves the issues pointed out by GCC.

I needed to specify the longer format specifier and cast some values to long (or even long long) values to address values that may be promoted to 64-bit values during the patch.
(For now, actually, the values are ketp at 32-bit, but as soon as a few data types are converted to 64-bit, we need the format string changes.)

4095 is the length of buffer minus one that is passed to sprintf before. Now we use snprintf().
I think I subtracted one from the real length just in case, we may overstep the buffer. Can't recall the details now.

This is unrelated to sign vs unsigned issue, but should be taken for security reasons as well.

This patch properly casts the signed vs unsigned comparison (which often causes subtle bug).
I made my GCC setting to flag them as errors because in some subdirectory of the parent ../../mozilla directory, such checking was enabled some years ago, and suddenly my patch I produced locally would not compile on try-comm-central. Ever since, I enabled the checking, but then some files under OTHER directories do not compile any more due to the check. comm/ldap is one such directory.

The errors observed during the compilation are as follows.
This patch should be a no brainer.

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c: In function ‘ber_get_tag’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:73:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
   73 |   for (i = 1; i < sizeof(ber_int_t); i++) {
      |                 ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c: In function ‘ber_skip_tag’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:126:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
  126 |     if (noctets > sizeof(ber_uint_t)) return (LBER_DEFAULT);
      |                 ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c: In function ‘ber_getnint’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:163:11: error: comparison of integer expressions of different signedness: ‘ber_slen_t’ {aka ‘int’} and ‘long unsigned int’ [-Werror=sign-compare]
  163 |   if (len > sizeof(ber_slen_t)) return (-1);
      |           ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c: In function ‘ber_scanf’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:434:41: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  434 |   for (rc = 0, p = (char*)fmt; *p && rc != LBER_DEFAULT; p++) {
      |                                         ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:500:18: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  500 |              tag != LBER_DEFAULT && tag != LBER_END_OF_SEQORSET &&
      |                  ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:500:41: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  500 |              tag != LBER_DEFAULT && tag != LBER_END_OF_SEQORSET &&
      |                                         ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:501:17: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  501 |              rc != LBER_DEFAULT;
      |                 ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:529:16: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  529 |         if (rc != LBER_DEFAULT && tag != LBER_END_OF_SEQORSET) {
      |                ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:529:39: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  529 |         if (rc != LBER_DEFAULT && tag != LBER_END_OF_SEQORSET) {
      |                                       ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:542:18: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  542 |              tag != LBER_DEFAULT && tag != LBER_END_OF_SEQORSET &&
      |                  ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:542:41: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  542 |              tag != LBER_DEFAULT && tag != LBER_END_OF_SEQORSET &&
      |                                         ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:543:17: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  543 |              rc != LBER_DEFAULT;
      |                 ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:564:16: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  564 |         if (rc != LBER_DEFAULT && tag != LBER_END_OF_SEQORSET) {
      |                ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:564:39: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  564 |         if (rc != LBER_DEFAULT && tag != LBER_END_OF_SEQORSET) {
      |                                       ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:573:44: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  573 |         if ((rc = ber_skip_tag(ber, &len)) == LBER_DEFAULT) break;
      |                                            ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:598:10: error: comparison of integer expressions of different signedness: ‘ber_int_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
  598 |   if (rc == LBER_DEFAULT) {
      |          ^~
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:597: decode.o] Error 1
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/liblber'
gmake[3]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/recurse.mk:72: comm/ldap/c-sdk/libraries/liblber/target-objects] Error 2
gmake[3]: *** Waiting for unfinished jobs....

          ...

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libprldap/ldappr-io.c: In function ‘prldap_poll’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libprldap/ldappr-io.c:269:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
  269 |       for (j = 0; j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
      |                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libprldap/ldappr-io.c:288:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
  288 |       for (j = 0; j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
      |                     ^
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:597: ldappr-io.o] Error 1
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/libprldap'
gmake[3]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/recurse.mk:72: comm/ldap/c-sdk/libraries/libprldap/target-objects] Error 2

          ...

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/error.c:394:61: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  394 |     if ((berrc = ber_scanf(&ber, "{iaa", &errcode, &m, &e)) != LBER_ERROR) {
      |                                                             ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/error.c:409:15: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  409 |     if (berrc != LBER_ERROR) {
      |               ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/error.c:423:19: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  423 |         if (berrc != LBER_ERROR &&
      |                   ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/error.c:431:15: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  431 |     if (berrc != LBER_ERROR && serverctrlsp != NULL &&
      |               ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/error.c:432:40: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  432 |         (berrc = ber_scanf(&ber, "}")) != LBER_ERROR) {
      |                                        ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/error.c:437:13: error: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  437 |   if (berrc == LBER_ERROR && err == LDAP_SUCCESS) {
      |             ^~
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:597: error.o] Error 1
gmake[4]: *** Waiting for unfinished jobs....

          ...


comm/ldap/xpcom/src/nsLDAPControl.o
/usr/bin/ccache /usr/bin/g++-10 -std=gnu++17 -o nsLDAPConnection.o -c  -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/stl_wrappers -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/system_wrappers -include /NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DMOZ_PREF_EXTENSIONS -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/xpcom/src -I/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/xpcom/src -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/nspr -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /NEW-SSD/moz-obj-dir/objdir-tb3/mozilla-config.h -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++2a-compat -Wduplicated-cond -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wno-multistatement-macros -Wno-error=class-memaccess -Wno-error=deprecated-copy -Wformat -Wformat-overflow=2 -Wno-psabi -fno-sized-deallocation -fno-aligned-new -fno-builtin-strlen -Wl,--gdb-index -Dfdatasync=fdatasync -DDEBUG_4GB_CHECK -DUSEHELGRIND=1 -DUSEVALGRIND=1 -DDEBUG -g -gsplit-dwarf -Werror=sign-compare -Werror=unused-result -Werror=unused-vbariable -Werror=format -fuse-ld=gold -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -g -Og -fvar-tracking -gdwarf-4 -fvar-tracking-assignments -freorder-blocks -fno-omit-frame-pointer -funwind-tables  -MD -MP -MF .deps/nsLDAPConnection.o.pp  -fdiagnostics-color  /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/xpcom/src/nsLDAPConnection.cpp
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/geteffectiverightsctrl.c: In function ‘ldap_create_geteffectiveRights_control’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/geteffectiverightsctrl.c:89:18: error: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Werror=sign-compare]
   89 |   if (LBER_ERROR == ber_printf(ber, "{s{v}}", authzid, attrlist)) {
      |                  ^~
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:597: geteffectiverightsctrl.o] Error 1
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/libldap'
gmake[3]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/recurse.mk:72: comm/ldap/c-sdk/libraries/libldap/target-objects] Error 2
comm/ldap/xpcom/src/nsLDAPMessage.o

After the previous patch is applied I get the following warnings and an error.
This one takes care of the error at the end.

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c: In function ‘ber_get_next’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:785:10: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  785 |     if (((ber_len_t)ber->ber_end - (ber_len_t)ber->ber_buf) < newlen) {
      |          ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:785:36: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  785 |     if (((ber_len_t)ber->ber_end - (ber_len_t)ber->ber_buf) < newlen) {
      |                                    ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:804:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  804 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |            ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:804:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  804 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |                                      ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:807:14: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  807 |       *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |              ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:807:42: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  807 |       *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |                                          ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:825:10: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  825 |   *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |          ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:825:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  825 |   *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |                                      ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c: In function ‘ber_get_next_buffer_ext’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1427:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1427 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |            ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1427:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1427 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |                                      ^
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/liblber'
gmake[4]: Entering directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/libldap'

          ...

comm/ldap/c-sdk/libraries/libldap/message.o
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘ldap_memcache_createkey’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:86:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   86 | #define NSLDAPI_SAFE_STRLEN(s) ((s) ? strlen((s)) + 1 : 1)
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:591:9: note: in expansion of macro ‘NSLDAPI_SAFE_STRLEN’
  591 |   len = NSLDAPI_SAFE_STRLEN(buf) + NSLDAPI_SAFE_STRLEN(tmpbase) +
      |         ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:85:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   85 | #define NSLDAPI_STR_NONNULL(s) ((s) ? (s) : "")
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:623:11: note: in expansion of macro ‘NSLDAPI_STR_NONNULL’
  623 |           NSLDAPI_STR_NONNULL(buf));
      |           ^~~~~~~~~~~~~~~~~~~
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:45:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_adj_size’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:912:36: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  912 |     assert(cache->ldmemc_size_used >= 0);
      |                                    ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:912:36: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  912 |     assert(cache->ldmemc_size_used >= 0);
      |                                    ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_access’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1451:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1451 |     int scope = (int)pData2;
      |                 ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_flush’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1557:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1557 |                       (void*)scope, NULL);
      |                       ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1559:64: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1559 |       memcache_access(cache, MEMCACHE_ACCESS_FLUSH, (void*)dn, (void*)scope,
      |                                                                ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘msgid_hashf’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1717:19: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1717 |   unsigned code = (unsigned)((ldapmemcacheReqId*)key)->ldmemcrid_ld;
      |                   ^

      ...

comm/ldap/c-sdk/libraries/libldap/proxyauthctrl.o
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c: In function ‘nsldapi_os_connect_with_to’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:360:65: warning: pointer targets in passing argument 5 of ‘getsockopt’ differ in signedness [-Wpointer-sign]
  360 |     if (getsockopt(sockfd, SOL_SOCKET, SO_ERROR, (char*)&error, &len) < 0)
      |                                                                 ^~~~
      |                                                                 |
      |                                                                 int *
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/ldap-int.h:61,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:67:
/usr/include/x86_64-linux-gnu/sys/socket.h:210:32: note: expected ‘socklen_t * restrict’ {aka ‘unsigned int * restrict’} but argument is of type ‘int *’
  210 |          socklen_t *__restrict __optlen) __THROW;
      |          ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

      ...

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c: In function ‘nsldapi_try_each_host’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:568:37: error: comparison of integer expressions of different signedness: ‘nsldapi_in_addr_t’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
  568 |     if ((address = inet_addr(host)) == -1) {
      |                                     ^~
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:597: os-ip.o] Error 1

After the previous patch is applied, then I get the following error.

This patches solves the error.

/usr/bin/ccache /usr/bin/g++-10 -std=gnu++17 -o Unified_cpp_mozglue_baseprofiler0.o -c  -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/stl_wrappers -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/system_wrappers -include /NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DIMPL_MFBT -DMOZ_VTUNE_INSTRUMENTATION -DIMPL_MFBT -DMOZ_HAS_MOZGLUE -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/mozglue/baseprofiler -I/NEW-SSD/moz-obj-dir/objdir-tb3/mozglue/baseprofiler -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/mozglue/baseprofiler/core -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/mozglue/linker -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/nspr -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /NEW-SSD/moz-obj-dir/objdir-tb3/mozilla-config.h -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++2a-compat -Wduplicated-cond -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wno-multistatement-macros -Wno-error=class-memaccess -Wno-error=deprecated-copy -Wformat -Wformat-overflow=2 -Wno-psabi -fno-sized-deallocation -fno-aligned-new -fno-builtin-strlen -Wl,--gdb-index -Dfdatasync=fdatasync -DDEBUG_4GB_CHECK -DUSEHELGRIND=1 -DUSEVALGRIND=1 -DDEBUG -g -gsplit-dwarf -Werror=sign-compare -Werror=unused-result -Werror=unused-variable -Werror=format -fuse-ld=gold -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -g -Og -fvar-tracking -gdwarf-4 -fvar-tracking-assignments -freorder-blocks -fno-omit-frame-pointer -funwind-tables -Wno-error=shadow -Wno-ignored-qualifiers  -MD -MP -MF .deps/Unified_cpp_mozglue_baseprofiler0.o.pp  -fdiagnostics-color  Unified_cpp_mozglue_baseprofiler0.cpp
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/ldap.h:47,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/ldap-int.h:87,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/utf8.c:39:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/utf8.c: In function ‘ldap_utf8strtok_r’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/ldap-extension.h:816:71: error: operand of ‘?:’ changes signedness from ‘char’ to ‘long unsigned int’ due to unsignedness of other operand [-Werror=sign-compare]
  816 |   ((0x80 & *(unsigned char*)(s)) ? ldap_utf8getcc((const char**)&s) : *s++)
      |                                                                       ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/utf8.c:185:8: note: in expansion of macro ‘LDAP_UTF8GETC’
  185 |   sc = LDAP_UTF8GETC(sp);
      |        ^~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/ldap-extension.h:814:57: error: operand of ‘?:’ changes signedness from ‘char’ to ‘long unsigned int’ due to unsignedness of other operand [-Werror=sign-compare]
  814 |   ((0x80 & *(unsigned char*)(s)) ? ldap_utf8getcc(&s) : *s++)
      |                                                         ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/utf8.c:186:24: note: in expansion of macro ‘LDAP_UTF8GETCC’
  186 |   for (bp = brk; (bc = LDAP_UTF8GETCC(bp)) != 0;) {
      |                        ^~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/ldap-extension.h:816:71: error: operand of ‘?:’ changes signedness from ‘char’ to ‘long unsigned int’ due to unsignedness of other operand [-Werror=sign-compare]
  816 |   ((0x80 & *(unsigned char*)(s)) ? ldap_utf8getcc((const char**)&s) : *s++)
      |                                                                       ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/utf8.c:200:10: note: in expansion of macro ‘LDAP_UTF8GETC’
  200 |     sc = LDAP_UTF8GETC(sp);
      |          ^~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/ldap-extension.h:814:57: error: operand of ‘?:’ changes signedness from ‘char’ to ‘long unsigned int’ due to unsignedness of other operand [-Werror=sign-compare]
  814 |   ((0x80 & *(unsigned char*)(s)) ? ldap_utf8getcc(&s) : *s++)
      |                                                         ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/utf8.c:203:17: note: in expansion of macro ‘LDAP_UTF8GETCC’
  203 |       if ((bc = LDAP_UTF8GETCC(bp)) == sc) {
      |                 ^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:597: utf8.o] Error 1
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/libldap'
gmake[3]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/recurse.mk:72: comm/ldap/c-sdk/libraries/libldap/target-objects] Error 2
gmake[3]: *** Waiting for unfinished jobs....

After the previous patch was applied, the build proceeds with warnings below.
Because the previous patch touches a header file, some files were re-compiled, amd some more warnings were printed (unused variables, etc.)

Now, after a few years of thinking of about the patches and a fresh look at the source code, though,
I decided to change |ber_tag_t| to |int|.

This is after I thought hard about the signed vs unsigned extension issue.

• A few functions return |ber_tag_t| as the next tag read from input stream.
• A few functions return the length of a string or something to that effect.
  However, upon error, these functions return 0xFFFFFFFF to signal an error.
  Defining |ber_tag_t| to be |int| causes a tricky issue of sign extension for these length returning functions, but still I felt it would be more advantageous for code cleanup reasons to do so.
  Thus I defined this AFTER all these years having created the modifications without doing so.

Anyway here is the patch.

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:86:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   86 | #define NSLDAPI_SAFE_STRLEN(s) ((s) ? strlen((s)) + 1 : 1)
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:591:9: note: in expansion of macro ‘NSLDAPI_SAFE_STRLEN’
  591 |   len = NSLDAPI_SAFE_STRLEN(buf) + NSLDAPI_SAFE_STRLEN(tmpbase) +
      |         ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:85:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   85 | #define NSLDAPI_STR_NONNULL(s) ((s) ? (s) : "")
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:623:11: note: in expansion of macro ‘NSLDAPI_STR_NONNULL’
  623 |           NSLDAPI_STR_NONNULL(buf));
      |           ^~~~~~~~~~~~~~~~~~~
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:45:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_adj_size’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:912:36: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  912 |     assert(cache->ldmemc_size_used >= 0);
      |                                    ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:912:36: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  912 |     assert(cache->ldmemc_size_used >= 0);
      |                                    ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_access’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1451:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1451 |     int scope = (int)pData2;
      |                 ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_flush’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1557:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1557 |                       (void*)scope, NULL);
      |                       ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1559:64: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1559 |       memcache_access(cache, MEMCACHE_ACCESS_FLUSH, (void*)dn, (void*)scope,
      |                                                                ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘msgid_hashf’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1717:19: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1717 |   unsigned code = (unsigned)((ldapmemcacheReqId*)key)->ldmemcrid_ld;
      |                   ^

      ...


/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c: In function ‘nsldapi_os_connect_with_to’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:360:65: warning: pointer targets in passing argument 5 of ‘getsockopt’ differ in signedness [-Wpointer-sign]
  360 |     if (getsockopt(sockfd, SOL_SOCKET, SO_ERROR, (char*)&error, &len) < 0)
      |                                                                 ^~~~
      |                                                                 |
      |                                                                 int *
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/ldap-int.h:61,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:67:
/usr/include/x86_64-linux-gnu/sys/socket.h:210:32: note: expected ‘socklen_t * restrict’ {aka ‘unsigned int * restrict’} but argument is of type ‘int *’
  210 |          socklen_t *__restrict __optlen) __THROW;
      |          ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~


      ...

 -MP -MF .deps/tmplout.o.pp  -fdiagnostics-color  /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c
comm/ldap/c-sdk/libraries/libldap/ufn.o
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘do_entry2text_search’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:395:20: warning: variable ‘html’ set but not used [-Wunused-but-set-variable]
  395 |   int err, freedn, html;
      |                    ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘searchaction’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:920:38: warning: variable ‘selectname’ set but not used [-Wunused-but-set-variable]
  920 |   char *value, *filtpattern, *attr, *selectname;
      |                                      ^~~~~~~~~~

Applying previous patch causes a few errors to show up aside from the existing warnings.

This patch is actually orthogonal to the errors here.

The patch deals with a function that returns a length of a buffer/string.
Originally, I thought I would return size_t for these functions by declaring |ber_len_t| to be |size_t|.
However, it signals an error by returning it (oxFFFFFFFF).
The error code was cast to |ber_tag_t|.
I changed the type of |ber_tag_t| to be |int| in my previous patch.
So now the error code CONSTANT becomes (-1).
Returning (-1) for the function that is supposed to return a length requires the type of the result to allow for minus one.
So, this patch tries to do so by declaring such functions to return |ber_slen_t| instead of |ber_len_t|.
In the older patches, I defined |ber_slen_t| to |ssize_t|, but for now, I keep |ber_slen_t| to be "int"
I have given up on fixing 64-bit cleanness issue for now. That will come later.

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c: In function ‘ber_get_next’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:762:45: error: comparison of integer expressions of different signedness: ‘ber_len_t’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
  762 |     if ((newlen = read_len_in_ber(sb, ber)) == LBER_DEFAULT) {
      |                                             ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:785:10: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  785 |     if (((ber_len_t)ber->ber_end - (ber_len_t)ber->ber_buf) < newlen) {
      |          ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:785:36: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  785 |     if (((ber_len_t)ber->ber_end - (ber_len_t)ber->ber_buf) < newlen) {
      |                                    ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:804:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  804 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |            ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:804:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  804 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |                                      ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:807:14: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  807 |       *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |              ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:807:42: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  807 |       *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |                                          ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:825:10: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  825 |   *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |          ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:825:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  825 |   *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |                                      ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c: At top level:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1246:22: error: conflicting types for ‘ber_get_next_buffer’
 1246 | ber_uint_t LDAP_CALL ber_get_next_buffer(void* buffer, size_t buffer_size,
      |                      ^~~~~~~~~~~~~~~~~~~
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/lber-int.h:119,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:51:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/lber.h:309:11: note: previous declaration of ‘ber_get_next_buffer’ was here
  309 | LDAP_CALL ber_get_next_buffer(void* buffer, size_t buffer_size, ber_len_t* len,
      |           ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1269:22: error: conflicting types for ‘ber_get_next_buffer_ext’
 1269 | ber_uint_t LDAP_CALL ber_get_next_buffer_ext(void* buffer, size_t buffer_size,
      |                      ^~~~~~~~~~~~~~~~~~~~~~~
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/lber-int.h:119,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:51:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/lber.h:312:11: note: previous declaration of ‘ber_get_next_buffer_ext’ was here
  312 | LDAP_CALL ber_get_next_buffer_ext(void* buffer, size_t buffer_size,
      |           ^~~~~~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c: In function ‘ber_get_next_buffer_ext’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1427:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1427 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |            ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1427:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1427 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |                                      ^
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:596: io.o] Error 1
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/liblber'
gmake[3]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/recurse.mk:72: comm/ldap/c-sdk/libraries/liblber/target-objects] Error 2
gmake[3]: *** Waiting for unfinished jobs....

          ...

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘ldap_memcache_createkey’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:86:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   86 | #define NSLDAPI_SAFE_STRLEN(s) ((s) ? strlen((s)) + 1 : 1)
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:591:9: note: in expansion of macro ‘NSLDAPI_SAFE_STRLEN’
  591 |   len = NSLDAPI_SAFE_STRLEN(buf) + NSLDAPI_SAFE_STRLEN(tmpbase) +
      |         ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:85:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   85 | #define NSLDAPI_STR_NONNULL(s) ((s) ? (s) : "")
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:623:11: note: in expansion of macro ‘NSLDAPI_STR_NONNULL’
  623 |           NSLDAPI_STR_NONNULL(buf));
      |           ^~~~~~~~~~~~~~~~~~~
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:45:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_adj_size’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:912:36: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  912 |     assert(cache->ldmemc_size_used >= 0);
      |                                    ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:912:36: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  912 |     assert(cache->ldmemc_size_used >= 0);
      |                                    ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_access’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1451:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1451 |     int scope = (int)pData2;
      |                 ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_flush’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1557:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1557 |                       (void*)scope, NULL);
      |                       ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1559:64: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1559 |       memcache_access(cache, MEMCACHE_ACCESS_FLUSH, (void*)dn, (void*)scope,
      |                                                                ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘msgid_hashf’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1717:19: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1717 |   unsigned code = (unsigned)((ldapmemcacheReqId*)key)->ldmemcrid_ld;
      |                   ^

      ...

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c: In function ‘nsldapi_os_connect_with_to’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:360:65: warning: pointer targets in passing argument 5 of ‘getsockopt’ differ in signedness [-Wpointer-sign]
  360 |     if (getsockopt(sockfd, SOL_SOCKET, SO_ERROR, (char*)&error, &len) < 0)
      |                                                                 ^~~~
      |                                                                 |
      |                                                                 int *
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/ldap-int.h:61,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:67:
/usr/include/x86_64-linux-gnu/sys/socket.h:210:32: note: expected ‘socklen_t * restrict’ {aka ‘unsigned int * restrict’} but argument is of type ‘int *’
  210 |          socklen_t *__restrict __optlen) __THROW;
      |          ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

After the previous patch is applied,
I get the following errors and warnings.

This patch tries to solve the errors.

gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/mozglue/build'
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:653:18: error: conflicting types for ‘get_ber_len’
  653 | static ber_len_t get_ber_len(BerElement* ber) {
      |                  ^~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:108:19: note: previous declaration of ‘get_ber_len’ was here
  108 | static ber_slen_t get_ber_len(BerElement* ber);
      |                   ^~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:677:18: error: conflicting types for ‘read_len_in_ber’
  677 | static ber_len_t read_len_in_ber(Sockbuf* sb, BerElement* ber) {
      |                  ^~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:109:19: note: previous declaration of ‘read_len_in_ber’ was here
  109 | static ber_slen_t read_len_in_ber(Sockbuf* sb, BerElement* ber);
      |                   ^~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c: In function ‘ber_get_next’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:796:16: error: comparison of integer expressions of different signedness: ‘ber_slen_t’ {aka ‘int’} and ‘ber_len_t’ {aka ‘unsigned int’} [-Werror=sign-compare]
  796 |         newlen > sb->sb_max_incoming) {
      |                ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:820:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  820 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |            ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:820:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  820 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |                                      ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:823:14: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  823 |       *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |              ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:823:42: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  823 |       *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |                                          ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:841:10: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  841 |   *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |          ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:841:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  841 |   *len = (ber_len_t)ber->ber_rwptr - (ber_len_t)orig_rwptr;
      |                                      ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c: At top level:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1262:22: error: conflicting types for ‘ber_get_next_buffer’
 1262 | ber_uint_t LDAP_CALL ber_get_next_buffer(void* buffer, size_t buffer_size,
      |                      ^~~~~~~~~~~~~~~~~~~
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/lber-int.h:119,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:51:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/lber.h:309:11: note: previous declaration of ‘ber_get_next_buffer’ was here
  309 | LDAP_CALL ber_get_next_buffer(void* buffer, size_t buffer_size, ber_len_t* len,
      |           ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1285:22: error: conflicting types for ‘ber_get_next_buffer_ext’
 1285 | ber_uint_t LDAP_CALL ber_get_next_buffer_ext(void* buffer, size_t buffer_size,
      |                      ^~~~~~~~~~~~~~~~~~~~~~~
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/lber-int.h:119,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:51:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/include/lber.h:312:11: note: previous declaration of ‘ber_get_next_buffer_ext’ was here
  312 | LDAP_CALL ber_get_next_buffer_ext(void* buffer, size_t buffer_size,
      |           ^~~~~~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c: In function ‘ber_get_next_buffer_ext’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1443:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1443 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |            ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/io.c:1443:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1443 |   toread = (ber_len_t)ber->ber_end - (ber_len_t)ber->ber_rwptr;
      |                                      ^
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:597: io.o] Error 1

With the previous patch, the build succeeds.

However, finally, I touch one of the 64-bit unclean part.
Namely, the hash function which did not handle 64-bit address very well is modified to use long long scalar so that the hash working on 64-bit address would work better.

One other part which is not 64-bit clean is so difficult to handle (there would be a cascade effect of
modifying function declartions/prototype), I am holding off the fix until the rest of the patch is adopted.
But at least I mark it with a comment.

(In reply to ISHIKAWA, Chiaki from comment #60)

Created attachment 9208646 [details] [diff] [review]
brand-new-1243121-ldap-part-6.5-memcache.patch

With the previous patch, the build succeeds.

However, finally, I touch one of the 64-bit unclean part.
Namely, the hash function which did not handle 64-bit address very well is modified to use long long scalar so that the hash working on 64-bit address would work better.

One other part which is not 64-bit clean is so difficult to handle (there would be a cascade effect of
modifying function declartions/prototype), I am holding off the fix until the rest of the patch is adopted.
But at least I mark it with a comment.

int scope = pData2;             /* pointer to scaler */

This is such a serious offender against 64-bit cleanness.
I wonder if the code works as expected in 64-bit build at all.
Oops, I notice an misspell.

(In reply to ISHIKAWA, Chiaki from comment #60)

Created attachment 9208646 [details] [diff] [review]
brand-new-1243121-ldap-part-6.5-memcache.patch

With the previous patch, the build succeeds.

However, finally, I touch one of the 64-bit unclean part.
Namely, the hash function which did not handle 64-bit address very well is modified to use long long scalar so that the hash working on 64-bit address would work better.

One other part which is not 64-bit clean is so difficult to handle (there would be a cascade effect of
modifying function declartions/prototype), I am holding off the fix until the rest of the patch is adopted.
But at least I mark it with a comment.

The cast was not quite correct, and I fixed it.
Also, I marked two other places where 64-bit cleanness is broken.
I fixed the misspell I mention in the previous comment.

Updated patch.

This eliminates the following warning visible in comment 55 and comment 57.

NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c: In function ‘nsldapi_os_connect_with_to’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:360:65: warning: pointer targets in passing argument 5 of ‘getsockopt’ differ in signedness [-Wpointer-sign]
  360 |     if (getsockopt(sockfd, SOL_SOCKET, SO_ERROR, (char*)&error, &len) < 0)
      |                                                                 ^~~~
      |                                                                 |
      |                                                                 int *
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/ldap-int.h:61,
                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/os-ip.c:67:
/usr/include/x86_64-linux-gnu/sys/socket.h:210:32: note: expected ‘socklen_t * restrict’ {aka ‘unsigned int * restrict’} but argument is of type ‘int *’
  210 |          socklen_t *__restrict __optlen) __THROW;
      |          ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

This patch annotates the two unused variables.
I wonder if the output the function(s) are supposed to create is correct or not without the use of the values in the vairables.

The warnings of the unsed variables are visible here.

      ...

 -MP -MF .deps/tmplout.o.pp  -fdiagnostics-color  /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c
comm/ldap/c-sdk/libraries/libldap/ufn.o
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘do_entry2text_search’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:395:20: warning: variable ‘html’ set but not used [-Wunused-but-set-variable]
  395 |   int err, freedn, html;
      |                    ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘searchaction’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:920:38: warning: variable ‘selectname’ set but not used [-Wunused-but-set-variable]
  920 |   char *value, *filtpattern, *attr, *selectname;
      |                                      ^~~~~~~~~~

With the previous three patches applied, I get the following warnings and the build proceeds.

This patch finally address the -Wtype-limit warnings that caught my eyes many , many months ago.

One prototype modification was necessitated during the change (that may become necessary due to later patch.) To be frank, I forgot why it became necessary at this stage.

gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/mfbt'
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘ldap_memcache_createkey’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:86:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   86 | #define NSLDAPI_SAFE_STRLEN(s) ((s) ? strlen((s)) + 1 : 1)
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:591:9: note: in expansion of macro ‘NSLDAPI_SAFE_STRLEN’
  591 |   len = NSLDAPI_SAFE_STRLEN(buf) + NSLDAPI_SAFE_STRLEN(tmpbase) +
      |         ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:85:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   85 | #define NSLDAPI_STR_NONNULL(s) ((s) ? (s) : "")
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:623:11: note: in expansion of macro ‘NSLDAPI_STR_NONNULL’
  623 |           NSLDAPI_STR_NONNULL(buf));
      |           ^~~~~~~~~~~~~~~~~~~
In file included from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:45:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_adj_size’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:912:36: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  912 |     assert(cache->ldmemc_size_used >= 0);
      |                                    ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:912:36: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
  912 |     assert(cache->ldmemc_size_used >= 0);
      |                                    ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_access’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1452:17: warning: initialization of ‘int’ from ‘void *’ makes integer from pointer without a cast [-Wint-conversion]
 1452 |     int scope = pData2;             /* pointer to scalar */
      |                 ^~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_flush’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1559:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1559 |                       (void*)scope, NULL);
      |                       ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1562:64: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1562 |       memcache_access(cache, MEMCACHE_ACCESS_FLUSH, (void*)dn, (void*)scope,
      |                                                                ^

      ...

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘do_entry2text_search’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:395:20: warning: variable ‘html’ set but not used [-Wunused-but-set-variable]
  395 |   int err, freedn, html;
      |                    ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘searchaction’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:921:38: warning: variable ‘selectname’ set but not used [-Wunused-but-set-variable]
  921 |   char *value, *filtpattern, *attr, *selectname;
      |                                      ^~~~~~~~~~

With the previous patch applied, I only got the following warnings. Things are looking good.

This patch looks at the somewhat loose handling of types in the file decode.c
The necessity of the change may not be obvious right now.
This is because I am not going aggressively toward 64-bit cleanness.
But I once define |ber_len_t| to |size_t| and |ber_slen_t| to |ssize_t|, and the compiler warnings pointed out dubious type casts and so forth.
The modifications were necessary to eliminate warnings.
(I missed some casts and such which are fixed in a later patch in the series.)

Somebody might want to go over the code by looking at the following explanation of
BER encoding to make sure this will become 64-bit clean. At least, I think I have make THIS PART 64-bit clean more or less with additional later patch
https://ldap.com/ldapv3-wire-protocol-reference-asn1-ber/

gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/third_party/bzip2'
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘ldap_memcache_createkey’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:86:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   86 | #define NSLDAPI_SAFE_STRLEN(s) ((s) ? strlen((s)) + 1 : 1)
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:591:9: note: in expansion of macro ‘NSLDAPI_SAFE_STRLEN’
  591 |   len = NSLDAPI_SAFE_STRLEN(buf) + NSLDAPI_SAFE_STRLEN(tmpbase) +
      |         ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:85:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   85 | #define NSLDAPI_STR_NONNULL(s) ((s) ? (s) : "")
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:623:11: note: in expansion of macro ‘NSLDAPI_STR_NONNULL’
  623 |           NSLDAPI_STR_NONNULL(buf));
      |           ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_access’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1453:17: warning: initialization of ‘int’ from ‘void *’ makes integer from pointer without a cast [-Wint-conversion]
 1453 |     int scope = pData2;             /* pointer to scalar */
      |                 ^~~~~~
gmake[4]: Entering directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/third_party/json-c'
gmake[4]: Nothing to be done for 'target-objects'.
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/third_party/json-c'
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/libldap'

With the previous patch, which I thought was OK when |ber_len_t| was |size_t| and |ber_slen_t| was either |signed long| or |sszie_t|, the build now produces following warnings.

I think I either modified the previous incorrectly during the transition to |ber_tag_t| to |int| or something.
Anyway, the pointer pun is very tricky.

This patch tries to address the issue.

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c: In function ‘ber_scanf’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:457:13: warning: pointer targets in assignment from ‘ber_slen_t *’ {aka ‘int *’} to ‘ber_len_t *’ {aka ‘unsigned int *’} differ in signedness [-Wpointer-sign]
  457 |         llp = va_arg(ap, ber_slen_t *);
      |             ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:458:31: warning: pointer targets in passing argument 2 of ‘ber_get_int’ differ in signedness [-Wpointer-sign]
  458 |         rc = ber_get_int(ber, llp);
      |                               ^~~
      |                               |
      |                               ber_len_t * {aka unsigned int *}
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:183:61: note: expected ‘ber_int_t *’ {aka ‘int *’} but argument is of type ‘ber_len_t *’ {aka ‘unsigned int *’}
  183 | ber_tag_t LDAP_CALL ber_get_int(BerElement* ber, ber_int_t* num) {
      |                                                  ~~~~~~~~~~~^~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:423:9: warning: variable ‘s’ set but not used [-Wunused-but-set-variable]
  423 |   char *s, **ss, ***sss;
      |         ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:473:14: warning: ‘ss’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  473 |         rc = ber_get_bitstringa(ber, ss, (ber_len_t *)sllp);
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/liblber'
gmake[4]: Entering directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/mailnews/compose/src'

This patch actually is no longer necessary now that
I reverted |ber_len_t| from |size_t| to |unsigned|, and |ber_slen_t| fro |sslen_t| to |int|,
and define |ber_tag_t| to |int|.

With all the patches including the previous patch applied in this order, the build proceeds with the following warning.

However, for completeness sake, I am showing the patch so that what type of tweaks would become necessary when 64-bit cleanness is pursued and |ber_len_t| and |ber_slen_t| may be redefined, etc.

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c: In function ‘ber_scanf’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/liblber/decode.c:423:9: warning: variable ‘s’ set but not used [-Wunused-but-set-variable]
  423 |   char *s, **ss, ***sss;
      |         ^

      ...

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘ldap_memcache_createkey’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:86:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   86 | #define NSLDAPI_SAFE_STRLEN(s) ((s) ? strlen((s)) + 1 : 1)
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:591:9: note: in expansion of macro ‘NSLDAPI_SAFE_STRLEN’
  591 |   len = NSLDAPI_SAFE_STRLEN(buf) + NSLDAPI_SAFE_STRLEN(tmpbase) +
      |         ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:85:37: warning: the address of ‘buf’ will always evaluate as ‘true’ [-Waddress]
   85 | #define NSLDAPI_STR_NONNULL(s) ((s) ? (s) : "")
      |                                     ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:623:11: note: in expansion of macro ‘NSLDAPI_STR_NONNULL’
  623 |           NSLDAPI_STR_NONNULL(buf));
      |           ^~~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_access’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1453:17: warning: initialization of ‘int’ from ‘void *’ makes integer from pointer without a cast [-Wint-conversion]
 1453 |     int scope = pData2;             /* pointer to scalar */
      |                 ^~~~~~

      ...

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘do_entry2text_search’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:395:20: warning: variable ‘html’ set but not used [-Wunused-but-set-variable]
  395 |   int err, freedn, html;
      |                    ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘searchaction’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:921:38: warning: variable ‘selectname’ set but not used [-Wunused-but-set-variable]
  921 |   char *value, *filtpattern, *attr, *selectname;
      |                                      ^~~~~~~~~~

      ...

(In reply to Rob Lemley [:rjl] from comment #49)

(In reply to ISHIKAWA, Chiaki from comment #47)

OTOH, I could finally run the build with my tentative patches WITHOUT causing ldap-related errors as in
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=96563a67f215e33cd770f5658ff9be4b3d00b3e5
and so maybe I am done with this bugzilla for now with those patches and aim for 64-bit cleanness in a different bugzilla.

What do people think?

I think that's a good idea. Breaking up the work you're doing into smaller pieces makes it easier for the reviewer(s).

You might want to rebase again, I believe that some of those test failures have been fixed.

I have posted the series of patches as I explained.
The reason I wanted to explain the errors/warning messages is that
some did not see why I needed to modify the source as I did before.
(See Comment 1. Actually, the original coder did not realize, say, assert() was meaningless and thus I wanted to check if the code is sane in terms of signedness of some variables, fields, etc.)

Anyway, the patches applied up to "Bug 1243121 part-6: casting pointer difference to ber_len_t" builds on try-comm-central and does not show any ldap-related errors anymore(!).

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=0c8b0f63c2edfcf05280117f9d039f94ebe49693

The patches applied up to "Bug 1243121 part-30.5 patch to fix a couple additional bugs/warnings from part-30" builds on try-comm-central and does not show any ldap-related errors anymore(!)
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=4f405d3478c26eb945c2e662e4b57ae7aa378af7

(Maybe as suggested "I believe that some of those test failures have been fixed." might have been fixed lately???)

Anyway, if each small patches are verified, I can consolidate them into a few large patches for easier handling.

Please recall there is still 64-bit cleanness issue.
The warnings below show the problem. A pointer is stored into |int| value and then that value is cast back to a pointer.
Nightmare in 64-bit build.

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1451:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 1451 |     int scope = (int)pData2;
      |                 ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c: In function ‘memcache_flush’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1557:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1557 |                       (void*)scope, NULL);
      |                       ^
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/memcache.c:1559:64: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
 1559 |       memcache_access(cache, MEMCACHE_ACCESS_FLUSH, (void*)dn, (void*)scope,
      |                         

I wonder if anyone uses 64-bit version of TB in an institutional setting where LDAP is used via address book at all.

I have NOT touched on the issue in this bugzilla because I have figured out that major surgery would be necessary to change the function prototype, etc. once I decide to extend the size of |scope| to a larger value. There will be a chain reaction of function declaration changes, etc.
That would mask the first set of patches that are necessary to make ldap code free of signed vs unsigned comparison issues (that is now flagged by my local setting of GCC. Actually thanks to the analysis, I believe I have solved a nasty problem of a function trying to return a minus-one (0xFFFFFFFF) as an unsigned error value, but would have failed once we move to 64-bit world and the length that returns can exceed 0xFFFFFFFFu.

Oh well. This code is old.

(In reply to Ben Campbell from comment #71)

Comment on attachment 9208622 [details] [diff] [review]
brand-new-1243121-ldap-part-1-simple.patch

Review of attachment 9208622 [details] [diff] [review]:

LGTM. A few nits about types, but all trivial.
Fills me with joy to see compiler warnings removed!

::: ldap/c-sdk/libraries/liblber/decode.c
@@ +497,5 @@

     j = 0;
     array_size = 0;
     for (tag = ber_first_element(ber, &len, &last);

•         tag != (int)LBER_DEFAULT && tag != (int)LBER_END_OF_SEQORSET &&
  

•         rc != (int)LBER_DEFAULT;
  

In this function, tag is defined as ber_int_t (about line 419).
But it is used exclusively as ber_tag_t. So the casts here and further
down could be avoided by changing the declaration instead.

::: ldap/c-sdk/libraries/libldap/error.c
@@ +391,5 @@

if (NSLDAPI_LDAP_VERSION(ld) < LDAP_VERSION2) {
berrc = ber_scanf(&ber, "{ia}", &errcode, &e);
} else {

• if ((berrc = ber_scanf(&ber, "{iaa", &errcode, &m, &e)) !=

•    (int)LBER_ERROR) {
  

Again here. berrc is defined as int (line 347), but is always used as a
ber_tag_t, so could avoid the casts by correcting it's original
declaration.

::: ldap/c-sdk/libraries/libldap/getoption.c
@@ +415,5 @@

   aip->ldapai_vendor_name = NULL;
   return (LDAP_NO_MEMORY);
 }

• for (i = 0; (unsigned)i < NSLDAPI_EXTENSIONS_COUNT; ++i) {

I think size_t is probably more technically correct than unsigned here
(although no real practical difference).

@@ +446,5 @@

 fip->ldapaif_info_version = LDAP_FEATURE_INFO_VERSION;
 return (LDAP_PARAM_ERROR);

}

• for (i = 0; (unsigned)i < NSLDAPI_EXTENSIONS_COUNT; ++i) {

size_t

@@ +453,5 @@

   break;
 }

}

• return (((unsigned)i < NSLDAPI_EXTENSIONS_COUNT) ? LDAP_SUCCESS

size_t

::: ldap/c-sdk/libraries/libprldap/ldappr-io.c
@@ +265,5 @@

   pds[i].fd = PRLDAP_GET_PRFD(fds[i].lpoll_socketarg);
 }
 pds[i].in_flags = pds[i].out_flags = 0;
 if (fds[i].lpoll_fd >= 0) {

•  for (j = 0; (unsigned)j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
  

size_t

@@ +284,5 @@

/* populate LDAP info. based on NSPR results */
for (i = 0; i < nfds; ++i) {
if (pds[i].fd != NULL) {

•  for (j = 0; (unsigned int) j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
  

size_t

So shall I create the new patch with the suggestion of explicit changes.?
Well, I will do anyway and post a new patch.

This incorporates the suggestion of benc and carrying over r++.

This is the necessary change to accommodate the modification in part-1 of the patch.

(In reply to Ben Campbell from comment #75)

Comment on attachment 9208620 [details] [diff] [review]
This handles the format string and snprintf buffer size issue.

Review of attachment 9208620 [details] [diff] [review]:

With the 'long long' casts, what size are you aiming for?
I have to admit I had no idea what size it was until I wrote a little test
program. Under gcc on my 64bit Linux system I get:

$ ./a.out 
sizeof(int): 4
sizeof(long int): 8
sizeof(long long int): 8
sizeof(__int128_t): 16
sizeof(bool): 8

I'd guess on a 32bit gcc, long int and bool would both be 32bits and the
rest would stay the same. I'd try on clang too, but it seems like I don't
have a stand-alone clang setup. But I'd expect it to be the same as gcc.

If you're aiming for 64bit values even on 32bit platforms, maybe it'd be
better to use uint64_t or int64_t, along with the PRIu64/PRIi64
printf specifiers
(https://en.wikipedia.org/wiki/Printf_format_string#Length_field)

::: ldap/c-sdk/libraries/libldap/getfilter.c
@@ +350,5 @@

     break;

   case '(':
   case ')':

•    sprintf(x, "\\%02x", (unsigned char)*v);
  

Wasn't the original 'unsigned' cast correct?
'%02x' expects an unsigned int, not char (according to wikipedia anyway:
https://en.wikipedia.org/wiki/Printf_format_string#Type_field ).

::: ldap/c-sdk/libraries/libldap/tmplout.c
@@ +617,5 @@

       }
       if (html) {
         sprintf(buf, "<DD>%s<BR>%s", p, eol);
       } else {

•        snprintf(buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol);
  

It's a little hard to tell, but I think the buffer used here is always
LDAP_DTMPL_BUFSIZ bytes? But hat's only certain if a null buf is passed in.
Without knowing the length for certain, is it even worth trying to use
snprintf()? If you have the length wrong, then it's probably no better than
a raw sprintf() :-(

In any case, if snprintf truncates the output, the buffer is left without
a '\0' terminator, which you'd have to deal with (either by bailing out with
an error code, re-allocating the buffer and trying again, or by manually
adding a nul-terminator and proceeding with a truncated string).

I will come back to your comment. Today, I am afraid I have caught cold.
This is rather indepndent patch.
I now request a few more reviews of series of the patches. They need to come after the part-1 patch.
TIA

(In reply to ISHIKAWA, Chiaki from comment #76)

(In reply to Ben Campbell from comment #75)

Comment on attachment 9208620 [details] [diff] [review]
This handles the format string and snprintf buffer size issue.

Review of attachment 9208620 [details] [diff] [review]:

With the 'long long' casts, what size are you aiming for?
I have to admit I had no idea what size it was until I wrote a little test
program. Under gcc on my 64bit Linux system I get:

$ ./a.out 
sizeof(int): 4
sizeof(long int): 8
sizeof(long long int): 8
sizeof(__int128_t): 16
sizeof(bool): 8

I'd guess on a 32bit gcc, long int and bool would both be 32bits and the
rest would stay the same. I'd try on clang too, but it seems like I don't
have a stand-alone clang setup. But I'd expect it to be the same as gcc.

If you're aiming for 64bit values even on 32bit platforms, maybe it'd be
better to use uint64_t or int64_t, along with the PRIu64/PRIi64
printf specifiers
(https://en.wikipedia.org/wiki/Printf_format_string#Length_field)

Right, I should use more standardized size today.
As I mentioned back in Comment 37, there was a time, a window of 10 days or two weeks when the standard macro symbols somehow mysteriously could not be understood by then MS compiler (I think it had prevoiusly), so I had to
resort to these kludges. I wanted to extend them as wide as possible for later modification.

I will fix them in the next round.

::: ldap/c-sdk/libraries/libldap/getfilter.c
@@ +350,5 @@

     break;

   case '(':
   case ')':

•    sprintf(x, "\\%02x", (unsigned char)*v);
  

Wasn't the original 'unsigned' cast correct?
'%02x' expects an unsigned int, not char (according to wikipedia anyway:
https://en.wikipedia.org/wiki/Printf_format_string#Type_field ).

• I have to be careful if "unsigned char" is universally supported, but that said,
  actually, the original coding is a bit misleading or too complex.
  As it turns out, we want to print the ascii value of '(', or ')' which is stored in *v (v being a pointer to char).
  I originally thought here *v might contain a octet whose MSB may be set to 1.
  That is why I wanted to make sure unwanted sign extension from char does not occur. (Come to think of it,
  is the conversion "char -> unsigned char -> unsigned int " instead of "char -> int -> unsigned int"? If so the original is OK.
  In any case, since *v is either '(' or ')', it is OK to leave it as it was.
  Or we could be a more explicit by saying,

sprintf(x, "\\%02x", (unsigned char)*v);
===> 
strncpy(x, *v == '(' ? "\\28" : "\\29", 4);

I say the above, because in the case of '' and '*', the code basically explicitly uses the string with the intended character code.
Maybe I would go with this.

cf. This part handles the escaping mechanism of RFC4515.
https://tools.ietf.org/html/rfc4515
From "3. String Search Filter Definition", fourth paragraph.

The <valueencoding> rule ensures that the entire filter string is a
   valid UTF-8 string and provides that the octets that represent the
   ASCII characters "*" (ASCII 0x2a), "(" (ASCII 0x28), ")" (ASCII
   0x29), "\" (ASCII 0x5c), and NUL (ASCII 0x00) are represented as a
   backslash "\" (ASCII 0x5c) followed by the two hexadecimal digits
   representing the value of the encoded octet.

::: ldap/c-sdk/libraries/libldap/tmplout.c
@@ +617,5 @@

       }
       if (html) {
         sprintf(buf, "<DD>%s<BR>%s", p, eol);
       } else {

•        snprintf(buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol);
  

It's a little hard to tell, but I think the buffer used here is always
LDAP_DTMPL_BUFSIZ bytes? But hat's only certain if a null buf is passed in.
Without knowing the length for certain, is it even worth trying to use
snprintf()? If you have the length wrong, then it's probably no better than
a raw sprintf() :-(

In any case, if snprintf truncates the output, the buffer is left without
a '\0' terminator, which you'd have to deal with (either by bailing out with
an error code, re-allocating the buffer and trying again, or by manually
adding a nul-terminator and proceeding with a truncated string).

I get your point.
I don't want to mess with seemingly working code as is.
However, the latest GCC compiler in its wisdom tries to point out
problems in format strings and many times it is correct.
Unless I somehow placate the compiler by using snprintf() I can't even compile.
(And since GCC is often correct, I don't want to suppress the warning/error here.)

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘do_entry2text_search’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:395:20: warning: variable ‘html’ set but not used [-Wunused-but-set-variable]
  395 |   int err, freedn, html;
      |                    ^~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘searchaction’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:921:38: warning: variable ‘selectname’ set but not used [-Wunused-but-set-variable]
  921 |   char *value, *filtpattern, *attr, *selectname;
      |                                      ^~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘output_dn’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:750:19: error: ‘%-*s’ directive output between 1 and 2147483647 bytes may exceed minimum required size of 4095 [-Werror=format-overflow=]
  750 |     sprintf(buf, "%-*s", width, " ");
      |                   ^~~~          ~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c: In function ‘do_vals2text’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:666:25: error: ‘%-*s’ directive output between 1 and 2147483648 bytes may exceed minimum required size of 4095 [-Werror=format-overflow=]
  666 |           sprintf(buf, "%-*s%s%s%-*s%s%s", labelwidth, " ", s, eol,
      |                         ^~~~                           ~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:666:24: note: assuming directive output of 3 bytes
  666 |           sprintf(buf, "%-*s%s%s%-*s%s%s", labelwidth, " ", s, eol,
      |                        ^~~~~~~~~~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:666:24: note: assuming directive output of 1 byte
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:666:24: note: assuming directive output of 40 bytes
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:666:24: note: assuming directive output of 1 byte
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:622:27: error: ‘%-*s’ directive output between 1 and 2147483648 bytes may exceed minimum required size of 4095 [-Werror=format-overflow=]
  622 |             sprintf(buf, "%-*s%s%s", labelwidth, " ", p, eol);
      |                           ^~~~                   ~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:622:26: note: assuming directive output of 40 bytes
  622 |             sprintf(buf, "%-*s%s%s", labelwidth, " ", p, eol);
      |                          ^~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:622:26: note: assuming directive output of 1 byte
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:681:23: error: ‘%-*s’ directive output between 1 and 2147483648 bytes may exceed minimum required size of 4095 [-Werror=format-overflow=]
  681 |         sprintf(buf, "%-*s%s%s", labelwidth, " ", outval, eol);
      |                       ^~~~                   ~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:681:22: note: assuming directive output of 4 bytes
  681 |         sprintf(buf, "%-*s%s%s", labelwidth, " ", outval, eol);
      |                      ^~~~~~~~~~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/ldap/c-sdk/libraries/libldap/tmplout.c:681:22: note: assuming directive output of 1 byte
cc1: some warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:596: tmplout.o] Error 1
gmake[4]: Leaving directory '/NEW-SSD/moz-obj-dir/objdir-tb3/comm/ldap/c-sdk/libraries/libldap'

Hmm, interesting. GCC seems to be capable of deducing the length of |buf| and other strings at compile time if possible?

I will reorder the patch so that this sprintf() issue is handled in the end.
I need to think over sprintf() issue a bit more.

Thank you again for your comment.

I will come back to your comment. Today, I am afraid I have caught cold.
This is rather indepndent patch.
I now request a few more reviews of series of the patches. They need to come after the part-1 patch.
TIA

Note: we'll be working on ldap in js (bug 1696625) soon and once that's successful, rip out the whole ldap dir. So, unless there are serious problems, might not be time best spent to fix minor issues there...

(In reply to ISHIKAWA, Chiaki from comment #76)

(In reply to Ben Campbell from comment #75)

::: ldap/c-sdk/libraries/libldap/getfilter.c

•    sprintf(x, "\\%02x", (unsigned char)*v);
  

Wasn't the original 'unsigned' cast correct?
I originally thought here *v might contain a octet whose MSB may be set to 1.

Ahh, that makes sense! The new (unsigned char) cast should work fine - I just found it odd that it was being cast down to a byte to be passed into a varargs fn, which takes everything as int sized params (I think).
I'd stay with (unsigned char).

Hmm, interesting. GCC seems to be capable of deducing the length of |buf| and other strings at compile time if possible?
Very odd - I guess all the uses of that buffer are within the same file/compilation unit, so gcc just knows that it's always at least 4095 bytes? Sometimes I think compilers are coded with pixie dust and magical unicorn burps, rather than C :-)

I will reorder the patch so that this sprintf() issue is handled in the end.
I need to think over sprintf() issue a bit more.

Well, given comment #78, I think maybe just go with what you've got for now. Even if the snprintf is wrong, it's no worse than the existing sprintf, and it fixes compilation under gcc. Maybe just add a comment in, eg:

// Using snprintf to compile on gcc. The size of 'buf' isn't clear (see Bug 1243121).

(In reply to Magnus Melin [:mkmelin] from comment #78)

Note: we'll be working on ldap in js (bug 1696625) soon and once that's successful, rip out the whole ldap dir. So, unless there are serious problems, might not be time best spent to fix minor issues there...

I see the issues here are a few.
1 - compilation issue.
2 - compile time warning.
3 - not 64-bit clean.

What is the timeline of bug 1696625?
We may simply fix issue 1 and 2 quickly and forget about 3.

(In reply to Ben Campbell from comment #80)

(In reply to ISHIKAWA, Chiaki from comment #76)

(In reply to Ben Campbell from comment #75)

::: ldap/c-sdk/libraries/libldap/getfilter.c

•    sprintf(x, "\\%02x", (unsigned char)*v);
  

Wasn't the original 'unsigned' cast correct?
I originally thought here *v might contain a octet whose MSB may be set to 1.

Ahh, that makes sense! The new (unsigned char) cast should work fine - I just found it odd that it was being cast down to a byte to be passed into a varargs fn, which takes everything as int sized params (I think).
I'd stay with (unsigned char).

Hmm, interesting. GCC seems to be capable of deducing the length of |buf| and other strings at compile time if possible?
Very odd - I guess all the uses of that buffer are within the same file/compilation unit, so gcc just knows that it's always at least 4095 bytes? Sometimes I think compilers are coded with pixie dust and magical unicorn burps, rather than C :-)

I will reorder the patch so that this sprintf() issue is handled in the end.
I need to think over sprintf() issue a bit more.

Well, given comment #78, I think maybe just go with what you've got for now. Even if the snprintf is wrong, it's no worse than the existing sprintf, and it fixes compilation under gcc. Maybe just add a comment in, eg:

// Using snprintf to compile on gcc. The size of 'buf' isn't clear (see Bug 1243121).

Thank you, Ben. I think we will fix what we can do quickly and suppress warnings now.

(In reply to ISHIKAWA, Chiaki from comment #81)

What is the timeline of bug 1696625?

Probably will be fixed with the next 4 months. Depends a bit on other priorities...
So for 91 the old LDAP code will still be around, but if we're lucky with the timeline it's preffed off.

(In reply to Magnus Melin [:mkmelin] from comment #83)

(In reply to ISHIKAWA, Chiaki from comment #81)

What is the timeline of bug 1696625?

Probably will be fixed with the next 4 months. Depends a bit on other priorities...
So for 91 the old LDAP code will still be around, but if we're lucky with the timeline it's preffed off.

Thanks.

I think Benc and I will land easy patches to shut up warnings, etc. in about a week, then.

This handles the format string and snprintf buffer size issue.

It now reflects the comment from benc in commet 82.
Added the comment regarding GCC compilation.
(I ran it through clang-format since the comment was rather lengthy.
I may need to post a patch to re-run clang-format over ldap directory entirely because
clang-format or its configuration has diverged from its behavior clang-format had
when /ldap subdirectory was reformatted in the last year or so.
mach clang-format produces different output for files which I did not touch at all.)

Comment on attachment 9208631 [details] [diff] [review]
brand-new-1243121-ldap-part-5.5-ber_tag_t.patch

This patch and subsequent two patches need to be reviewed as one chunk.
They are explained in comment 57, comment 58 and comment 59.

Comment on attachment 9208633 [details] [diff] [review]
brand-new-1243121-ldap-part-5.75-ber_tag_t.patch

As I explained, this patch needs to be reviewed with two other patches.
(See comment 57, comment 58, comment 59).

Comment on attachment 9208644 [details] [diff] [review]
brand-new-1243121-ldap-part-6-pointer-local-OK.patch

This one needed to be viewed along with two previous patches.
I introduced ber_slen_t.
Also, the cast of pointer to an int is OK in 32-bit world, but not so in 64-bit world.
This is a one small step for 64-bit cleanliness although there is a serious offender which won't be fixed in a short time.

I forgot the types and format strings for the extended size issues in the printf.
Fixed them. I needed to include <inttypes.h>.

Comment on attachment 9212106 [details] [diff] [review]
brand-new-1243121-ldap-part-99-format-string-and-type.patch (take 2)

Doh. I just reviewed an obsolete patch, didn't I?

Comment on attachment 9212112 [details]
brand-new-1243121-ldap-part-0-format-string-and-type.patch (take 3)

The x = *v == '(' ? "\\28" : "\\29"; bit doesn't compile for me. It doesn't like trying to copy the literal strings into char x[4]. Changing x to a const char* instead of an array would probably work though.

Is this the right patch? It's also missing the gcc snprintf comments, and it doesn't seem to have the "inttypes.h" include you mention in comment 89...

Parts 5.5, 5.75 and 6 look good to me, but since they all have to be landed at once I've combined them into a single patch (and ran ./mach clang-format over it).

Still need to sort out part 0 before we can start landing them. I just kicked off a try run with part 0, some fiddling to get part 0 compiling, and this patch:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d0ba9839a2c83d6636babddc822277136e698968

Waaaay less compiler warnings with these changes. Yay!

Sorry, I uploaded an outdated patch.
This is the one I am using and submitted the job to tryserver.

Sorry for the confusion.

(In reply to Ben Campbell from comment #92)

Comment on attachment 9212112 [details]
brand-new-1243121-ldap-part-0-format-string-and-type.patch (take 3)

The x = *v == '(' ? "\\28" : "\\29"; bit doesn't compile for me. It doesn't like trying to copy the literal strings into char x[4]. Changing x to a const char* instead of an array would probably work though.

Is this the right patch? It's also missing the gcc snprintf comments, and it doesn't seem to have the "inttypes.h" include you mention in comment 89...

I uploaded a wrong one. In my comment above, I uploaded the correct patch. Sorry for the confusion.

Comment on attachment 9212467 [details]
brand-new-1243121-ldap-part-99-format-string-and-type.patch (take 4)

Ahh, that looks like the one!

But for some reason, it won't import cleanly into my tree...

$ hg pull
$ hg checkout comm
$ hg import "https://bug1243121.bmoattachments.org/attachment.cgi?id=9212467"
applying https://bug1243121.bmoattachments.org/attachment.cgi?id=9212467
patching file ldap/c-sdk/libraries/liblber/io.c
Hunk #1 FAILED at 44
Hunk #3 FAILED at 827
2 out of 3 hunks FAILED -- saving rejects to file ldap/c-sdk/libraries/liblber/io.c.rej
abort: patch failed to apply

Most of the patch applies just fine, but there are two hunks in ldap/c-sdk/libraries/liblber/io.c being rejected... (including the <inttypes.h> addition at the top!). I can't see any reason why those two hunks are being rejected. I'm really scratching my head here :-(
Does it apply cleanly for you on the latest code?

Oh, meant to add: it's probably worth changing the commit message - the "part 99" doesn't make much sense there. That kind of numbering is useful on the patch filenames here, so they can be landed in the right order (although I'm not sure 99 make sense as the first one ;- ).

(In reply to ISHIKAWA, Chiaki from comment #95)

(In reply to Ben Campbell from comment #92)

Comment on attachment 9212112 [details]
brand-new-1243121-ldap-part-0-format-string-and-type.patch (take 3)

The x = *v == '(' ? "\\28" : "\\29"; bit doesn't compile for me. It doesn't like trying to copy the literal strings into char x[4]. Changing x to a const char* instead of an array would probably work though.

Is this the right patch? It's also missing the gcc snprintf comments, and it doesn't seem to have the "inttypes.h" include you mention in comment 89...

I uploaded a wrong one. In my comment above, I uploaded the correct patch. Sorry for the confusion.

I am really sorry for the confusion.
I said in comment 77,

will reorder the patch so that this sprintf() issue is handled in the end.
I need to think over sprintf() issue a bit more.

In order to gain time for thinking about sprintf() a bit more (eventually we decided to go with what I have),
I put this patch AT THE END OF the queue.
So sorry for not mentioning it. Yeah, you noticed the numbering change. I moved this patch at the END of the series. That is why
you get the patch errors.

 8 A brand-new-1243121-ldap-part-1-simple.patch: Bug 1243121 part-1: eliminating signed vs uns...
 9 A brand-new-1243121-ldap-part-2-address-minusone-comparison.patch: Bug 1243121 part-2: comp...
10 A brand-new-1243121-ldap-part-4-different-sign-extension-in-a-macro-local_-OK.patch: Bug 12...
11 A brand-new-1243121-ldap-part-5.5-ber_tag_t.patch: Bug 1243121 part-5.5 make ber_tag_t to int
12 A brand-new-1243121-ldap-part-5.75-ber_tag_t.patch: Bug 1243121 part-5.75: LBER_DEFAULT sig...
13 A brand-new-1243121-ldap-part-6-pointer-local-OK.patch: Bug 1243121 part-6: casting pointer...
14 A brand-new-1243121-ldap-part-6.5-memcache.patch: Bug 1243121 part-6.5: handled 64-bit uncl...
15 A brand-new-1243121-ldap-part-8-os-ip.patch: Bug 1243121 part-8 patch to fix to eliminate w...
16 A brand-new-1243121-ldap-part-10-tmplout.patch: Bug 1243121 part-10 patch annotate unused v...
17 A brand-new-1243121-ldap-part-20-memcache.patch: Bug 1243121 part-20 patch for memcache
18 A brand-new-1243121-ldap-part-30-decode.c.patch: Bug 1243121 part-30: decode.c to accommodt...
19 A brand-new-1243121-ldap-part-30.5-decode.c.fix.patch: Bug 1243121 part-30.5 patch to fix a...
20 A brand-new-1243121-ldap-part-99-format-string-and-type.patch: Bug 1243121 part-99: format ...

That said, it would be too confusing. So I revert my local change to put the format string changes at the end of the series of patches.
I have modified the patch to be at the top again. (part-00 moniker.)
That necessitates the minor change to brand-new-1243121-ldap-parts-5.5-and-5.75-and-6-combined.patch (benc+) because the format strings are changed first now.
So let me upload the modified two patches.
Now my local queue looks like this. (I am going to upload the patches marked with *.)

* 8 A brand-new-1243121-ldap-part-00-format-string-and-type.patch: Bug 1243121 part-99: format string and snprintf buffer size issue
  9 A brand-new-1243121-ldap-part-1-simple.patch: Bug 1243121 part-1: eliminating signed vs unsigned warnings (now treated erro), first...
 10 A brand-new-1243121-ldap-part-2-address-minusone-comparison.patch: Bug 1243121 part-2: comparison of minus one vs pointer
 11 A brand-new-1243121-ldap-part-4-different-sign-extension-in-a-macro-local_-OK.patch: Bug 1243121 part-4 patch to fix different sign...
* 12 A brand-new-1243121-ldap-parts-5.5-and-5.75-and-6-combined.patch: Bug 1243121 - Fix ldap signed/unsigned compiler warnings. r=benc
 13 A brand-new-1243121-ldap-part-6.5-memcache.patch: Bug 1243121 part-6.5: handled 64-bit uncleanness
 14 A brand-new-1243121-ldap-part-8-os-ip.patch: Bug 1243121 part-8 patch to fix to eliminate warning for getsockopt().
 15 A brand-new-1243121-ldap-part-10-tmplout.patch: Bug 1243121 part-10 patch annotate unused variable assignment for future review
 16 A brand-new-1243121-ldap-part-20-memcache.patch: Bug 1243121 part-20 patch for memcache
 17 A brand-new-1243121-ldap-part-30-decode.c.patch: Bug 1243121 part-30: decode.c to accommodte the new better type defintion
 18 A brand-new-1243121-ldap-part-30.5-decode.c.fix.patch: Bug 1243121 part-30.5 patch to fix a couple additional bugs/warnings from pa...

I may throw in a clang-format patch at the end as I mentioned before.

Fix the patch position. Back to the beginning of the series.

Needed to change to accommodate the new part-00 patch (moved from the end of the series to the beginning again).

Corrected commit message to reflect the patch series position.

Try server run with the corrected sequence.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=5529002e35e751619eeae1ee7b2d30810bccc7ca

Note: especially when you have a series of patches, it would be preferable to use phabricator for reviews, and easier for you as well.

(In reply to Magnus Melin [:mkmelin] from comment #104)

Note: especially when you have a series of patches, it would be preferable to use phabricator for reviews, and easier for you as well.

I will try to get the hang of it.
This seems to be the minimal information.
https://wiki.mozilla.org/Phabricator
Is there any more detailed instruction on this. (I knew M-C patches seems to use phabricator, but the hurdle for occasional patch contributor was immense.)

It's quite nice nowadays.

./mach install-moz-phab

See https://github.com/mozilla-conduit/review and https://moz-conduit.readthedocs.io/en/latest/mozphab-linux.html

Once installed

• commit locally, submit current revision for review: moz-phab .
• download another revision and apply it to your current revision: (export DX=<phab D>) and moz-phab patch $DX --apply-to .

(In reply to Magnus Melin [:mkmelin] from comment #106)

It's quite nice nowadays.

./mach install-moz-phab

See https://github.com/mozilla-conduit/review and https://moz-conduit.readthedocs.io/en/latest/mozphab-linux.html

Once installed

• commit locally, submit current revision for review: moz-phab .
• download another revision and apply it to your current revision: (export DX=<phab D>) and moz-phab patch $DX --apply-to .

Thank you for the pointer.

Now it is my turn to modify locally crafted scripts (bash scripts mainly) to cope with this. : right now, it basically uses MQUEUE extension which is no longer mozilla favorite (I know).

I will study and see how it goes.

Thank you again.

How much of this goes away with js-ldap?

libldap is not used in js-ldap, will be removed after 91.

(In reply to Ping Chen (:rnons) from comment #109)

libldap is not used in js-ldap, will be removed after 91.

When will C-C tree be in shape for 91? I mean when will it be removed in C-C tree?

Depending on the length of wait, I may try again to push the change quickly because
this part of the code hinders the use of stricter checking by the compiler.
(I basically compile local tree by blanket -Wwarn=error using GCC, except for some specific warnings.
This sub-subdirectory needed the patch to pass the compiler.)

TIA

Not decided yet. It depends a bit how the larger feedback on js-ldap is.

This is no longer relevant. The C source code has been removed.