Deprecate SbStringFormat functions (youtube#1999)

Replace SbStringFormat*() calls with POSIX vsnprintf(). SbStringFormat -> vsnprintf SbStringFormatF -> snprintf SbStringFormatWide -> vswprintf SbStringFormatUnsafeF -> sprintf Backward compatibility is protected by "SB_API_VERSION < 16". Tested on Linux, Android, Evergreen(14,15,16), and Win32. b/307941391 Change-Id: I437ff3d7bf735b46767867ea844aeffb28e58b4d
Rongo-JL · Dec 19, 2023 · c0cf534 · c0cf534
1 parent b6551e1
commit c0cf534
Show file tree

Hide file tree

Showing 52 changed files with 124 additions and 123 deletions.
diff --git a/base/strings/string_util_starboard.h b/base/strings/string_util_starboard.h
@@ -16,6 +16,9 @@
 #define BASE_STRING_UTIL_STARBOARD_H_
 
 #include <stdarg.h>
+#if SB_API_VERSION >= 16
+#include <stdio.h>
+#endif
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
@@ -25,13 +28,9 @@
 
 namespace base {
 
-#if defined(vsnprintf)
-#undef vsnprintf
-#endif
-
 inline int vsnprintf(char* buffer, size_t size,
                      const char* format, va_list arguments) {
-  return SbStringFormat(buffer, size, format, arguments);
+  return ::vsnprintf(buffer, size, format, arguments);
 }
 
 inline int strncmp16(const char16* s1, const char16* s2, size_t count) {
@@ -42,12 +41,6 @@ inline int strncmp16(const char16* s1, const char16* s2, size_t count) {
 #endif
 }
 
-inline int vswprintf(wchar_t* buffer, size_t size,
-                     const wchar_t* format, va_list arguments) {
-  DCHECK(base::IsWprintfFormatPortable(format));
-  return SbStringFormatWide(buffer, size, format, arguments);
-}
-
 }  // namespace base
 
 #endif  // BASE_STRING_UTIL_STARBOARD_H_
diff --git a/base/strings/stringprintf_unittest.cc b/base/strings/stringprintf_unittest.cc
@@ -120,7 +120,7 @@ TEST(StringPrintfTest, Grow) {
   const int kRefSize = 320000;
   char* ref = new char[kRefSize];
 #if defined(STARBOARD)
-  SbStringFormatF(ref, kRefSize, fmt, src, src, src, src, src, src, src);
+  snprintf(ref, kRefSize, fmt, src, src, src, src, src, src, src);
 #else
 #if defined(OS_WIN)
   sprintf_s(ref, kRefSize, fmt, src, src, src, src, src, src, src);

diff --git a/base/trace_event/heap_profiler_allocation_context_tracker.cc b/base/trace_event/heap_profiler_allocation_context_tracker.cc
@@ -71,7 +71,7 @@ const char* GetAndLeakThreadName() {
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
   // Use tid if we don't have a thread name.
-  SbStringFormatF(name, sizeof(name), "%lu",
+  snprintf(name, sizeof(name), "%lu",
                   static_cast<unsigned long>(PlatformThread::CurrentId()));
   return strdup(name);
 }

diff --git a/cobalt/browser/memory_settings/pretty_print.cc b/cobalt/browser/memory_settings/pretty_print.cc
@@ -149,7 +149,7 @@ std::string ToMegabyteString(int64_t bytes, int decimal_places) {
   ss_fmt << "%." << decimal_places << "f MB";
 
   char buff[128];
-  SbStringFormatF(buff, sizeof(buff), ss_fmt.str().c_str(), megabytes);
+  snprintf(buff, sizeof(buff), ss_fmt.str().c_str(), megabytes);
   // Use 16
   return std::string(buff);
 }

diff --git a/net/cert/internal/trust_store_in_memory_starboard.cc b/net/cert/internal/trust_store_in_memory_starboard.cc
@@ -95,7 +95,7 @@ scoped_refptr<ParsedCertificate> TrustStoreInMemoryStarboard::TryLoadCert(
     const base::StringPiece& cert_name) const {
   auto hash = CertNameHash(cert_name.data(), cert_name.length());
   char cert_file_name[256];
-  SbStringFormatF(cert_file_name, 256, "%08lx.%d", hash, 0);
+  snprintf(cert_file_name, 256, "%08lx.%d", hash, 0);
 
   if (trusted_cert_names_on_disk_.find(cert_file_name) ==
       trusted_cert_names_on_disk_.end()) {

diff --git a/starboard/CHANGELOG.md b/starboard/CHANGELOG.md
@@ -9,6 +9,12 @@ since the version previous to it.
 
 ## Version 16
 
+### Deprecated SbStringFormat APIs and migrated to POSIX memory APIs
+The StringFormat management APIs `SbStringFormat`, `SbStringFormatF`,
+`SbStringFormatWide`, `SbStringFormatUnsifeF` are deprecated and the
+standard APIs `vsnprintf`, `vfnprintf`, `vswprintf`, `snprintf`
+from `<stdlib.h>` should be used instead.
+
 ### Deprecated SbMemory allocation APIs and migrated to POSIX memory APIs
 The memory management APIs `SbMemoryAllocate`, `SbMemoryReallocate`,
 `SbMemoryCalloc`, `SbMemoryAllocateAligned`, `SbMemoryDeallocate`,

diff --git a/starboard/android/shared/system_get_property.cc b/starboard/android/shared/system_get_property.cc
@@ -77,8 +77,8 @@ bool CopyAndroidPlatformName(char* out_value, int value_length) {
   }
 
   char result_string[kStringBufferSize];
-  SbStringFormatF(result_string, kStringBufferSize, kPlatformNameFormat,
-                  version_string_buffer);
+  snprintf(result_string, kStringBufferSize, kPlatformNameFormat,
+           version_string_buffer);
 
   return CopyStringAndTestIfSuccess(out_value, value_length, result_string);
 }

diff --git a/starboard/client_porting/poem/stdio_poem.h b/starboard/client_porting/poem/stdio_poem.h
@@ -24,16 +24,6 @@
 #include "starboard/memory.h"
 #include "starboard/string.h"
 
-// the following functions can have variable number of arguments
-// and, out of compatibility concerns, we chose to not use
-// __VA_ARGS__ functionality.
-#undef vsnprintf
-#define vsnprintf SbStringFormat
-#undef snprintf
-#define snprintf SbStringFormatF
-#undef sprintf
-#define sprintf SbStringFormatUnsafeF
-
 #endif  // POEM_NO_EMULATION
 
 #endif  // STARBOARD

diff --git a/starboard/client_porting/poem/wchar_poem.h b/starboard/client_porting/poem/wchar_poem.h
@@ -23,9 +23,6 @@
 
 #include "starboard/string.h"
 
-#undef vswprintf
-#define vswprintf SbStringFormatWide
-
 #endif  // POEM_NO_EMULATION
 
 #endif  // STARBOARD

diff --git a/starboard/common/string.h b/starboard/common/string.h
@@ -20,6 +20,9 @@
 #define STARBOARD_COMMON_STRING_H_
 
 #include <stdarg.h>
+#if SB_API_VERSION >= 16
+#include <stdio.h>
+#endif
 #include <cstring>
 #include <string>
 #include <vector>
@@ -35,7 +38,7 @@ SB_C_INLINE std::string FormatString(const char* format, ...)
 SB_C_INLINE std::string FormatString(const char* format, ...) {
   va_list arguments;
   va_start(arguments, format);
-  int expected_size = ::SbStringFormat(NULL, 0, format, arguments);
+  int expected_size = vsnprintf(NULL, 0, format, arguments);
   va_end(arguments);
 
   std::string result;
@@ -45,7 +48,7 @@ SB_C_INLINE std::string FormatString(const char* format, ...) {
 
   std::vector<char> buffer(expected_size + 1);
   va_start(arguments, format);
-  ::SbStringFormat(buffer.data(), buffer.size(), format, arguments);
+  vsnprintf(buffer.data(), buffer.size(), format, arguments);
   va_end(arguments);
   return std::string(buffer.data(), expected_size);
 }

diff --git a/starboard/doc/c99.md b/starboard/doc/c99.md
@@ -134,3 +134,11 @@ deprecated and eventually removed.
 * wmemmove
 * wmemset
 * wcsncmp
+* snprintf
+* sprintf
+* vfwprintf
+* vsnprintf
+* vswprintf
+* ferror
+* fputwc
+* fwide
diff --git a/starboard/elf_loader/exported_symbols.cc b/starboard/elf_loader/exported_symbols.cc
@@ -307,10 +307,8 @@ ExportedSymbols::ExportedSymbols() {
   REGISTER_SYMBOL(SbStringCompareNoCase);
   REGISTER_SYMBOL(SbStringCompareNoCaseN);
   REGISTER_SYMBOL(SbStringDuplicate);
-#endif  // SB_API_VERSION < 16
   REGISTER_SYMBOL(SbStringFormat);
   REGISTER_SYMBOL(SbStringFormatWide);
-#if SB_API_VERSION < 16
   REGISTER_SYMBOL(SbStringScan);
 #endif  // SB_API_VERSION < 16
   REGISTER_SYMBOL(SbSystemBreakIntoDebugger);
@@ -406,6 +404,20 @@ ExportedSymbols::ExportedSymbols() {
   REGISTER_SYMBOL(calloc);
   REGISTER_SYMBOL(posix_memalign);
   REGISTER_SYMBOL(free);
+  REGISTER_SYMBOL(sprintf);
+  REGISTER_SYMBOL(snprintf);
+  REGISTER_SYMBOL(vfwprintf);
+  REGISTER_SYMBOL(vsnprintf);
+#if defined(_MSC_VER)
+  // MSVC provides a template with the same name.
+  // The cast helps the compiler to pick the correct C function pointer to be
+  // used.
+  REGISTER_SYMBOL(
+      static_cast<int (*)(wchar_t* buffer, size_t count, const wchar_t* format,
+                          va_list argptr)>(vswprintf));
+#else
+  REGISTER_SYMBOL(vswprintf);
+#endif  // defined(_MSC_VER)
   REGISTER_SYMBOL(vsscanf);
 #endif  // SB_API_VERSION >= 16
 

diff --git a/starboard/linux/shared/system_get_path.cc b/starboard/linux/shared/system_get_path.cc
@@ -41,8 +41,7 @@ bool GetCacheDirectory(char* out_path, int path_size) {
                                                       kMaxPathSize)) {
     return false;
   }
-  int result =
-      SbStringFormatF(out_path, path_size, "%s/.cache", home_path.data());
+  int result = snprintf(out_path, path_size, "%s/.cache", home_path.data());
   if (result < 0 || result >= path_size) {
     out_path[0] = '\0';
     return false;
@@ -57,8 +56,8 @@ bool GetStorageDirectory(char* out_path, int path_size) {
                                                       kMaxPathSize)) {
     return false;
   }
-  int result = SbStringFormatF(out_path, path_size, "%s/.cobalt_storage",
-                               home_path.data());
+  int result =
+      snprintf(out_path, path_size, "%s/.cobalt_storage", home_path.data());
   if (result < 0 || result >= path_size) {
     out_path[0] = '\0';
     return false;
@@ -159,8 +158,8 @@ bool GetTemporaryDirectory(char* out_path, int path_size) {
     return false;
   }
 
-  int result = SbStringFormatF(out_path, path_size, "/tmp/%s-%d",
-                               binary_name.data(), static_cast<int>(getpid()));
+  int result = snprintf(out_path, path_size, "/tmp/%s-%d", binary_name.data(),
+                        static_cast<int>(getpid()));
   if (result < 0 || result >= path_size) {
     out_path[0] = '\0';
     return false;

diff --git a/starboard/loader_app/installation_manager.cc b/starboard/loader_app/installation_manager.cc
@@ -201,18 +201,17 @@ std::string InstallationManager::DumpInstallationSlots() {
   out << "size=";
   const int kBufSize = 50;
   char buf_num[kBufSize];
-  SbStringFormatF(buf_num, kBufSize, "%d",
-                  installation_store_.installations_size());
+  snprintf(buf_num, kBufSize, "%d", installation_store_.installations_size());
   out << buf_num;
 
   out << " roll_forward_to_installation=";
-  SbStringFormatF(buf_num, kBufSize, "%d",
-                  installation_store_.roll_forward_to_installation());
+  snprintf(buf_num, kBufSize, "%d",
+           installation_store_.roll_forward_to_installation());
   out << buf_num;
   out << ";";
   for (int i = 0; i < installation_store_.installations_size(); i++) {
     out << " installation_";
-    SbStringFormatF(buf_num, kBufSize, "%d", i);
+    snprintf(buf_num, kBufSize, "%d", i);
     out << buf_num;
     out << " is_successful=";
     if (installation_store_.installations(i).is_successful()) {
@@ -222,13 +221,13 @@ std::string InstallationManager::DumpInstallationSlots() {
     }
 
     out << " num_tries_left=";
-    SbStringFormatF(buf_num, kBufSize, "%d",
-                    installation_store_.installations(i).num_tries_left());
+    snprintf(buf_num, kBufSize, "%d",
+             installation_store_.installations(i).num_tries_left());
     out << buf_num;
 
     out << " priority=";
-    SbStringFormatF(buf_num, kBufSize, "%d",
-                    installation_store_.installations(i).priority());
+    snprintf(buf_num, kBufSize, "%d",
+             installation_store_.installations(i).priority());
     out << buf_num;
     out << ";";
   }
@@ -686,11 +685,11 @@ bool InstallationManager::GetInstallationPathInternal(int installation_index,
   // SLOT_0 is placed in |kSbSystemPathContentDirectory|, under the subdirectory
   // 'app/cobalt'.
   if (installation_index == 0) {
-    SbStringFormatF(path, path_length, "%s%s%s%s%s", content_dir_.c_str(),
-                    kSbFileSepString, "app", kSbFileSepString, "cobalt");
+    snprintf(path, path_length, "%s%s%s%s%s", content_dir_.c_str(),
+             kSbFileSepString, "app", kSbFileSepString, "cobalt");
   } else {
-    SbStringFormatF(path, path_length, "%s%s%s%d", storage_dir_.c_str(),
-                    kSbFileSepString, "installation_", installation_index);
+    snprintf(path, path_length, "%s%s%s%d", storage_dir_.c_str(),
+             kSbFileSepString, "installation_", installation_index);
   }
 
   return true;

diff --git a/starboard/loader_app/slot_management.cc b/starboard/loader_app/slot_management.cc
@@ -223,14 +223,13 @@ void* LoadSlotManagedLibrary(const std::string& app_key,
 
     // installation_n/lib/libcobalt.so
     std::vector<char> compressed_lib_path(kSbFileMaxPath);
-    SbStringFormatF(compressed_lib_path.data(), kSbFileMaxPath, "%s%s%s%s%s",
-                    installation_path.data(), kSbFileSepString,
-                    kCobaltLibraryPath, kSbFileSepString,
-                    kCompressedCobaltLibraryName);
+    snprintf(compressed_lib_path.data(), kSbFileMaxPath, "%s%s%s%s%s",
+             installation_path.data(), kSbFileSepString, kCobaltLibraryPath,
+             kSbFileSepString, kCompressedCobaltLibraryName);
     std::vector<char> uncompressed_lib_path(kSbFileMaxPath);
-    SbStringFormatF(uncompressed_lib_path.data(), kSbFileMaxPath, "%s%s%s%s%s",
-                    installation_path.data(), kSbFileSepString,
-                    kCobaltLibraryPath, kSbFileSepString, kCobaltLibraryName);
+    snprintf(uncompressed_lib_path.data(), kSbFileMaxPath, "%s%s%s%s%s",
+             installation_path.data(), kSbFileSepString, kCobaltLibraryPath,
+             kSbFileSepString, kCobaltLibraryName);
 
     std::string lib_path;
     bool use_compression;
@@ -258,9 +257,8 @@ void* LoadSlotManagedLibrary(const std::string& app_key,
     if (alternative_content_path.empty()) {
       // installation_n/content
       std::vector<char> content_path(kSbFileMaxPath);
-      SbStringFormatF(content_path.data(), kSbFileMaxPath, "%s%s%s",
-                      installation_path.data(), kSbFileSepString,
-                      kCobaltContentPath);
+      snprintf(content_path.data(), kSbFileMaxPath, "%s%s%s",
+               installation_path.data(), kSbFileSepString, kCobaltContentPath);
       content = content_path.data();
     } else {
       content = alternative_content_path.c_str();

diff --git a/starboard/nplb/BUILD.gn b/starboard/nplb/BUILD.gn
@@ -173,6 +173,8 @@ target(gtest_target_type, "nplb") {
     "string_compare_no_case_n_test.cc",
     "string_compare_no_case_test.cc",
     "string_duplicate_test.cc",
+
+    # TODO: b/307941391: test "SbStringFormatWideTest" is deprecated in SB16
     "string_format_test.cc",
     "string_format_wide_test.cc",
     "string_scan_test.cc",

diff --git a/starboard/nplb/string_format_test.cc b/starboard/nplb/string_format_test.cc
@@ -14,6 +14,7 @@
 
 // Here we are not trying to do anything fancy, just to really sanity check that
 // this is hooked up to something.
+#if SB_API_VERSION < 16
 
 #include "starboard/common/string.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -45,3 +46,5 @@ TEST(SbStringFormatTest, SunnyDay) {
 }  // namespace
 }  // namespace nplb
 }  // namespace starboard
+
+#endif  // SB_API_VERSION < 16
diff --git a/starboard/nplb/string_format_wide_test.cc b/starboard/nplb/string_format_wide_test.cc
@@ -14,6 +14,7 @@
 
 // Here we are not trying to do anything fancy, just to really sanity check that
 // this is hooked up to something.
+#if SB_API_VERSION < 16
 
 #include "starboard/common/string.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -48,3 +49,5 @@ TEST(SbStringFormatWideTest, SunnyDay) {
 }  // namespace
 }  // namespace nplb
 }  // namespace starboard
+
+#endif  // SB_API_VERSION < 16
diff --git a/starboard/shared/ffmpeg/ffmpeg_dynamic_load_dispatch_impl.cc b/starboard/shared/ffmpeg/ffmpeg_dynamic_load_dispatch_impl.cc
@@ -153,7 +153,7 @@ const int kMaxVersionedLibraryNameLength = 32;
 
 std::string GetVersionedLibraryName(const char* name, const int version) {
   char s[kMaxVersionedLibraryNameLength];
-  SbStringFormatF(s, kMaxVersionedLibraryNameLength, "%s.%d", name, version);
+  snprintf(s, kMaxVersionedLibraryNameLength, "%s.%d", name, version);
   return std::string(s);
 }
 

diff --git a/starboard/shared/posix/string_format.cc b/starboard/shared/posix/string_format.cc
@@ -17,9 +17,11 @@
 #include <stdarg.h>
 #include <stdio.h>
 
+#if SB_API_VERSION < 16
 int SbStringFormat(char* out_buffer,
                    size_t buffer_size,
                    const char* format,
                    va_list arguments) {
   return vsnprintf(out_buffer, buffer_size, format, arguments);
 }
+#endif
diff --git a/starboard/shared/posix/string_format_wide.cc b/starboard/shared/posix/string_format_wide.cc
@@ -18,9 +18,11 @@
 #include <stdio.h>
 #include <wchar.h>
 
+#if SB_API_VERSION < 16
 int SbStringFormatWide(wchar_t* out_buffer,
                        size_t buffer_size,
                        const wchar_t* format,
                        va_list arguments) {
   return vswprintf(out_buffer, buffer_size, format, arguments);
 }
+#endif
diff --git a/starboard/shared/starboard/link_receiver.cc b/starboard/shared/starboard/link_receiver.cc
@@ -293,7 +293,7 @@ void LinkReceiver::Impl::Run() {
   }
 
   char port_string[32] = {0};
-  SbStringFormatF(port_string, SB_ARRAY_SIZE(port_string), "%d", actual_port_);
+  snprintf(port_string, SB_ARRAY_SIZE(port_string), "%d", actual_port_);
   CreateTemporaryFile("link_receiver_port", port_string, strlen(port_string));
 
   if (!AddForAccept(listen_socket_.get())) {