Bug 4875 pt3: GCC-8 compile errors with -O3 optimization

[yadij]

Building with GCC-8 and the -O3 optimization level exposes
several more cases of -Werror -Wall turning warnings into hard
errors, and two actual cases of string truncation - one
harmless, the other possibly causing subtle errors.

Alter the fde::desc type from char* to SBuf. Allowing variable
length descriptions capable of holding modern URLs better and
SBuf optimized copy instead of full-length string copy between
buffers when the description is a constant.

[rousskov]

If you take the high road of improving the affected code, then I think this PR should be split into three independent PRs (some of which need fixes as detailed in inlined comments):

1. the strncat() replacement part can go as is (or slightly polished)
2. fde::desc PR (the redesign here would avoid the bugs introduced in this PR)
3. parseTimeLine() API fix (instead of the current workaround)

Otherwise, a single PR with a few small independent changes is acceptable, but fde changes (the second part) should be completely different (to avoid problems discussed in inlined comments for that part) -- they should work around the problem like the parseTimeLine() change currently does.

[rousskov]

IIRC, desc is only used for debugging/reporting. If possible, we should not add frequent (multiple times per transaction) memory allocations/deallocations for those purposes.

The old preallocated FD_DESC_SZ buffer approach was better than the PR version, but it can be improved further to avoid memory copies in most cases. If you want to improve it, consider changing desc type to SBuf while adjusting most fd_note() callers to supply statically allocated constant SBufs. SBuf was designed for this sort of thing.

[yadij]

61 bytes is very much insufficient to contain even modern domains, let alone the URL it is supposed to be reporting.

Changing it to SBuf was the first thing I tried. It had two problems;

• First that SBuf adds allocate/deallocate cycle same as this patch in addition to a secondary allocate/deallocate on each mgr report. You made it pretty clear in previous PR that touching those mgr reports even to fix typos is going to be vetoed unless the PR redesigns them wholesale and completely into absolutely perfect code.
• Second it makes everything in Squid depend on libsbuf. Including libraries libsbuf itself depends on. So we get circular reference in linker dependency checking. Here again the mgr reports (for SBuf ones) are going to need a redesign to remove that dependency.

The next thing I looked at was using static strings the desc member just pointed at. This has issues again with the URL cases where the content has to be dynamically assigned. We could work around that with a paired ID+string design for v5+, but that is too much change for this fix which needs to go into v4 stables.

[yadij]

NP: I'm not seeking to improve the desc handling, just to resolve the issue that several KB of URL string are being stuffed into 61B memory. Increasing FD_DESC_SZ is not the solution it appears to be as that would require many MB of RAM to be allocated and not used (most Squid already allocate over 4MB there and never use it).

[rousskov]

61 bytes is very much insufficient to contain even modern domains, let alone the URL it is supposed to be reporting.

Yes, of course. Nothing I said contradicts that AFAICT.

First that SBuf adds allocate/deallocate cycle same as this patch

This is incorrect. A proper implementation using SBuf will not add runtime allocations and deallocations. See my original comment for more details. Avoiding all allocations may be difficult, but not all problems have easy solutions, and perhaps we can make exceptions for some rare cases.

in addition to a secondary allocate/deallocate on each mgr report

Unlike collecting transaction information for possible future debugging/triage (discussed above), cache manager reports are allowed to have necessary additional allocations, of course. I doubt they would be necessary in this particular case, but if they are, they are not a blocking problem.

Second it makes everything in Squid depend on libsbuf

I suspect (and hope) that it is possible to avoid creating that side effect without huge changes. Clearly, most Squid code does not need access to the global file descriptor table where the FD descriptions are stored. Said that, a lot of Squid code will naturally depend on libsbuf regardless of this PR. There is no way around that because a lot of Squid code needs strings.

IIRC, some high-level code may need to be removed from libsbuf. There is nothing wrong with that.

I'm [just] seeking to resolve the issue that several KB of URL string are being stuffed into 61B memory.

Proper truncation seems to be a simple solution if you do not want to improve things beyond that.

Increasing FD_DESC_SZ is not the solution it appears to be as that would require many MB of RAM to be allocated and not used (most Squid already allocate over 4MB there and never use it).

FWIW, I do not necessarily see a one-time allocation of "many MB" as a showstopper, especially if it can be easily disabled via some #define for those rare cases where Squid is deployed on boxes where every few MB count. I would be more worried about copying those large buffers all the time, but that copying can be removed with a bit of work. It is certainly not the best long-term solution for preserving full FD descriptions, but we can discuss increasing the current buffer size a bit to lessen the pain.

---

Please note that I do not insist on using SBuf for FD descriptions. I think SBuf is the right solution for the "preserving full descriptions" problem, but there can be other, better solutions out there. The solution originally proposed by this PR is not one of them.

And if you do not want to solve that problem at all, then I certainly do not (and cannot) insist on finding a solution!

[rousskov]

Thank you for extracting the useful pieces from this PR into #287 and #288. Should this PR be closed now? If this PR is still useful, please adjust its description to match the adjusted content/intent.

[yadij]

I am leaving the FD related changes under this PR, so it is still relevant. I will get back to this when they are merged and the PR branch can be made cleaner.

[yadij]

Okay, updated now to use SBuf with most being constants instead of dynamic allocation.

[rousskov]

Please update PR title and description to reflect the current state of this PR. That state has changed a lot since those texts were written.

[yadij]

I adjusted the text right after this last commit. The driver behind this PR is still the GCC-8 build errors with the cause being char* string truncation warning promoted by -Wall to error. The fix is fde::desc member change to SBuf. (Reports in the bug about it being 'fixed' already were for Squid patched with earlier versions of this PR diff.).

[rousskov]

The driver behind this PR is still the GCC-8 build errors

Recording your motivation is often a good idea, but the PR description should focus on the changes. A reader should not be forced to ignore the PR/commit title and the first paragraph of the PR/commit description to finally find out what the PR is all about!

[rousskov]

I left a few suggestions, but please start with the change request marked [Blocker]. If we do not address that concern, the rest does not really matter. Moreover, the rest of the PR may be significantly affected by the changes required to address that concern.

[rousskov]

I do not think we should introduce this performance regression on a common code path. Better debugging/reporting is just not worth it. I have ideas on how this regression can be avoided (and debugging improved), but I do not want to waste time detailing and discussing them if you do not want to spend a lot more time on this PR. [Blocker]

[yadij]

Since this PR is about fixing the compile error I don't want to complicate it any further. But do want to hear these ideas. Can you post them to squid-dev so we can go over it separately please.

FYI, This is not much of a performance impact at all. The data-copy part cancels out with the old code removed. The addition is the "allocate" part - which for SBuf will almost always be from the short-strings pool. So just a matter of a few pointer operations instead of a regular memory alloc() syscall it would have been.

[rousskov]

do want to hear these ideas. Can you post them to squid-dev so we can go over it separately

Done at http://lists.squid-cache.org/pipermail/squid-dev/2019-February/009503.html

FYI, This is not much of a performance impact at all

FYI, "death by a thousand cuts" is one of the reasons "why we can't have nice things".

[yadij]

Closing as I no longer have GCC-8 to test this bug fix.