Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ siteupdate: Use of sprintf, deprecated on MacOS/XCode #585

Closed
jteresco opened this issue Mar 28, 2023 · 4 comments · Fixed by #632
Closed

C++ siteupdate: Use of sprintf, deprecated on MacOS/XCode #585

jteresco opened this issue Mar 28, 2023 · 4 comments · Fixed by #632

Comments

@jteresco
Copy link
Contributor

When compiling on MacOS, the C++ site update code generates pages of warnings:

siteupdate.cpp:527:4: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]
                        sprintf(fstr, ": %.2f mi", cr->mileage);
                        ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/stdio.h:188:1: note: 'sprintf' has been explicitly marked deprecated here
__deprecated_msg("This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead.")

I'm ok leaving it or replacing our sprintf calls with snprintf calls. A little of what I saw when searching around on this is that there are also some more C++-specific ways to achieve the same result.

Compiler version:

Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
@yakra
Copy link
Contributor

yakra commented Mar 31, 2023

A little of what I saw when searching around on this is that there are also some more C++-specific ways to achieve the same result.

I'll hazard a guess that these use std::string. I'm ambivalent toward std::string (vice C strings); on the one hand, it's great for permanent storage of text, or reading in new data when I don't know how much storage space I'll need. Calling size() is more convenient than either calling strlen or storing the size in a separate variable. They make good hash table keys; I don't have to worry about writing my own hash or pred functions; those are built in. Sometimes I'll use std::string when I'd otherwise want to use a C string based solution, because the alternative is Just Too Inconvenient (Waypoint simplification comes to mind, though exactly what part of it, I don't remember).
OTOH, if not used carefully, they can eat up a lot of RAM bandwidth and be a real performance-killer. Dialing back on std::string usage has yielded good speed improvements in graph generation and userlogs.
So if the code is to change here, it makes sense to use the C string framework already in place. A sprintf -> snprintf conversion would be a simple find-n-replace-all, sprintf(fstr -> snprintf(fstr, buflen, where buflen could be declared as a constexpr in each function if needed.
*Whines* Do I haaave to though?

/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/stdio.h:188:1:

This suggests to me this may be an Apple-specific thing (*glares at Apple*), although, clang 14 vs 13 on noreaster... wonder if this may show up on Unix-Unix boxes with new clang versions in the future? Meh.
*Climbs onto soapbox*
Yes, I know sprintf can be dangerous. As any good C programmer should. A char* is just a memory address, and on its own says nothing about how much buffer space we have available to write to. It's up to the programmer to know and accept the risk involved, and use the function responsibly. Which I believe siteupdate does. The buffer never changes from one char* to another. The format strings can't change; they're all hard-coded. All the formatted data are numbers (timestamps, lat/long coords, angles, ints, mileages & percentages). As for the amount of buffer space it can all take up, we're bounded to the right of the decimal point by the format string used. Never more places than %.17g. To the left, angles & percentages will never have > 3 digits. Lat/long coords can, sure, but that'll require multiple trips around the world in wptedit, trying to break stuff. As for ints, mileages, and percentages, I've made sure to allocate enough space for the max digits an int can hold, plus a - sign.

Consarnit, I'm trying to be a cowboy here, and Apple's excessive hand-holding ain't helping! 🤣

Rather than just a blanket SPRINTF BAD deprecation of the function, I'd even prefer GCC's approach of heuristically determining if an overflow will be likely, like what happened with 58b4b57.
To unpack that one for a moment:

char fstr[44];
sprintf(fstr, "(%.15g,%.15g)", w->lat, w->lng);

One byte has to be the null terminator, so that leaves us 43 B useful buffer space.
2x15 to the right of the . = 30, +5 for (.,.) = 35, +2 for - signs = 37. That leaves us 2x3 B to the left of the decimal point. Enough to record any angle (unless we're trying to break stuff). But I tossed it 4 B more anyway, just to quiet g++ down.

I see 2 ways to quiet XCode down:

  1. Add -Wno-deprecated-declarations to CXXFLAGS in the Makefile.
    • OT1H, this could mean missing out an other potentially important deprecations as well.
    • OTOH, what else is likely to become deprecated as long as we're using the C++11 standard?
    • OT3H, I'm not ruling out switching to a newer standard in the future. C++17's structured bindings and std::shared_mutex are tempting.
  2. Bite the bullet and switch to snprintf.
    • This would mean additional bounds checking to ensure that no more than n bytes are written. Noticeable performance degradation?
    • This means letting Apple win! 🤣

So I'll gladly accept @jteresco's low priority tag here, and sweep this under the rug while I pursue more interesting things near-term. Unless the warnings are annoying enough that you'd really like to see them gone, improving the S/N ratio for other compiler error/warnings that are more important.

@yakra
Copy link
Contributor

yakra commented Jul 30, 2023

fprintf may be able to get rid of most or even all of the sprintf calls.

The downside is that it uses C's FILE rather than C++'s std::ofstream, so it'd involve a little reinvention of the wheel.

@yakra
Copy link
Contributor

yakra commented May 10, 2024

@yakra
Copy link
Contributor

yakra commented May 12, 2024

Great news:

sprintf is a performance killer. Yes, this is great news.
Getting rid of it inner loops is the Magic Bullet for fixing poor FreeBSD performance. Including graph generation.

Most uses format a string and immediately plop it into a file.
Here, we can use ofstream::precision, occasionally setf and unsetf, and more traditional << operators.

I'm not going to look into fprintf as it surely has to suffer from the same drawbacks as sprintf, having to parse a format string.

  • graph generation
    This finally opens the floodgates & allows graph generation to scale to a higher number of threads on FreeBSD.
    As of 3282921 (yakra#251), lab2 (CentOS) was just shy of breaking the 3-second barrier.
    With this, it averages 1.0088 s, even managing 0.9985 on one run.
    FreeBSD even outperforms both CentOS and Ubuntu now.
    Caveats:
    • bsdlab has different RAM chips from lab3 & lab4, which may have different timings. I doubt this is the contributing factor, but worth noting. Heck, it could even be that bsdlab has slower timings.
    • Different clang versions are used; 9.0.1, 6.0.0 and 11.0.1 for lab3, lab4 and bsdlab respectively.)
  • sqlfile1
    sprintf is removed from sqlfile1 locally. I've done "after" benchmarks, but not "before" yet.
    As sqlfile1 is run in the background during graph generation, it made sense to convert this over too before benchmarking graphs.
    There was already no sprintf in sqlfile2.
  • userlogs
    Another task that scaled poorly under FreeBSD.
    Here are links to charts of how the last three performance improvements played out.
    This was slightly less straightworward: The format_clinched_mi function, which fomatted and returned a char* is gone, replaced by a lambda that captures std::ofstream log. Several of the long series of ofstream insertions had to be broken up into 2 or 3 separate commands.

ToDo:

  • near-miss points
    Similar to userlogs in that there's also Waypint::str() to contend with, also a good candidate to be replaced by a lambda. Shouldn't be too complicated.
  • routedatastats.log
    should be pretty straightforward.
  • stats CSVs
    likewise. There's a multithreaded stats CSV option, disabled by default because it performed so poorly, even on Linux. Getting rid of sprintf may change that.

The remaining uses do use the results for string manipulation, not just writing to a file. These cases are infrequent enough that I can just accept using snprintf instead; any performance will be negligible.

  • Flagging a handful of datachecks:
    LONG_SEGMENT, SHARP_ANGLE, OUT_OF_BOUNDS, VISIBLE_DISTANCE, DUPLICATE_COORDS,
  • ElapsedTime::et()
  • GraphListEntry::tag()
  • And finally, WaypointQuadtree::str(). I think there are no calls to this at all other than some commented-out debug stuff.

I'm going to hold off on a pull request for this for a wee bit.
yakra#251 is finally ready to go, so I want to gather up the stats, make some benchmark charts, type out the pull request text and get that in. If not later today, Tuesday.
LOL, after more than a month of perfecting that, this comes along, takes a few minutes to change a couple dozen lines of code, and has a far bigger impact! 😁

yakra added a commit to yakra/DataProcessing that referenced this issue May 19, 2024
jteresco added a commit that referenced this issue May 20, 2024
yakra added a commit to yakra/DataProcessing that referenced this issue May 28, 2024
jteresco added a commit that referenced this issue Jun 5, 2024
@yakra yakra mentioned this issue Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants