Update to latest tinyobjloader with patched tryParseDouble

[calderpg-tri]

This change is 

[calderpg-tri]

Working on anzu 6348 identified double parsing in tinyobjloader as a significant contributor to model rebuild time. This PR updates to the latest tinyobjloader with the body of homegrown double parsing tryParseDouble replaced with strtod. From callgrind on the model rebuild demo, tinyobj::LoadObj is down from 39% to 28% total cost, and double parsing down from 28% to 11% total cost.

Note that the changes to tinyobjloader involve the use of C99 functions, which are likely incompatible with tinyobjloader supporting C++03 and thus it seems unlikely that this patch could be upstreamed.

@SeanCurtis-TRI @jwnimmer-tri

[calderpg-tri]

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please

[calderpg-tri]

@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please

[calderpg-tri]

In related tinyobjloader news, there is apparently an actual documented vulnerability TALOS-2020-1212 which appears to correspond to placeholder CVE-2020-28589. No relevant commits to master since that issue, but there is a branch being worked on that suggests it could be a use-after-free bug.

[traversaro]

FYI @calderpg-tri the use of strtod may introduce problems due to the fact that strtod behavior depends on the process-global C locale, similarly to how the URDF parsing depended on the process-global C++ locale before the fix in #10326 .

x-ref : ros/urdfdom#98, robotology/idyntree#288

[calderpg-tri]

Given that any patches to tinyobjloader we make only need to work on a small number of up-to-date platforms, there are a couple alternatives we could use:

• strtod_l that takes a provided locale (although we'd need to create said locale somewhere longer-lived or lose any of the performance gains)
• Modify LoadObj to use

const locale_t parse_locale = newlocale(LC_NUMERIC_MASK, "C", (locale_t)0);
const locale_t previous_locale = uselocale(parse_locale);
...
uselocale(previous_locale);
freelocale(parse_locale);

[calderpg-tri]

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please

[calderpg-tri]

It looks like using strtod_l with a static locale_t doesn't add any expense over plain strtod.

[jwnimmer-tri]

From a build perspective, when we need to patch an external, my guidance for Drake is to keep pointing the external at upstream, but then add a patch file (using the patches = [] attribute to the github_archive) to specify our local diffs. We try not to point our externals at individuals' personal repositories.

[calderpg-tri]

Yes, once I settle on changes to tinyobjloader the patch route is the right way to go. I had originally hoped that an upstreamable patch might exist, but I doubt that is possible. Note that the repo needs to change from syoyo to tinyobjloader anyways, since the project was moved last year.

[calderpg-tri]

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please

[calderpg-tri]

+@SeanCurtis-TRI for feature review, thanks!

[SeanCurtis-TRI]

with some minor nits.

+@jwnimmer-tri for platform, please.

Reviewed 4 of 5 files at r1, 2 of 2 files at r3.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @calderpg-tri and @jwnimmer-tri)

---

geometry/render/gl_renderer/test/shape_meshes_test.cc, line 73 at r3 (raw file):

    DRAKE_EXPECT_THROWS_MESSAGE(
        LoadMeshFromObj(&in_stream), std::runtime_error,
        "tinyobj::LoadObj failed to load file.+");

nit: This makes me very sad. The old error message gave actionable feedback. Does the new message do the same? What is swallowed up in the .*? This definitely represents a regression and I'd like to be sure it's unavoidable.

At the very least, I'd like a TODO somewhere demanding investigation. Feel free to put my name on it.

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 7 at r3 (raw file):

 #include <string>
 #include <vector>
+#include <math.h>

nit: This looks unused.

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 8 at r3 (raw file):

 #include <vector>
+#include <math.h>
+#include <stdlib.h>

nit: Why not <cstdlib>?

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 13 at r3 (raw file):

+#include <xlocale.h>
+#else
+#include <locale.h>

nit: Why not <locale>?

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 22 at r3 (raw file):

     return false;
   }
+  

nit: trailing whitespace. (Sure there's other trailing whitspace in this file, but this is new to the patch.)

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 108 at r3 (raw file):

-    }
-  } else if (*curr == 'e' || *curr == 'E') {
+  errno = 0;

BTW Perhaps I'm being obtuse. But I couldn't find documentation that suggests that the errno value provides greater insight than isfinite. Do you have a link? And if it does, might be worth including the link here.

[jwnimmer-tri]

Ugh. I'm sad we don't have std::from_chars yet on 18.04.

platform.

Reviewed 4 of 5 files at r1, 2 of 2 files at r3.
Reviewable status: 6 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @calderpg-tri)

[SeanCurtis-TRI]

TIL std::from_chars. I need to write someone a sternly worded letter about that.

Reviewable status: 6 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @calderpg-tri)

[calderpg-tri]

As a general note, I used the C functions in the patch in the initial hope of producing something upstreamable. As written now, it needs C99 and would likely need broader locale handling for other platforms.

Reviewable status: 5 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

---

geometry/render/gl_renderer/test/shape_meshes_test.cc, line 73 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This makes me very sad. The old error message gave actionable feedback. Does the new message do the same? What is swallowed up in the .*? This definitely represents a regression and I'd like to be sure it's unavoidable.

At the very least, I'd like a TODO somewhere demanding investigation. Feel free to put my name on it.

The .* is swallowing up the filename. The error message is definitely less descriptive, but is primarily the consequence of LoadObj finally returning false in error cases. I'll add a TODO to shape_meshes.cc.

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 7 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This looks unused.

Used by isfinite

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 8 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Why not <cstdlib>?

C strtod appears to have slightly different semantics than std::strtod in certain range error cases.

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 13 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Why not <locale>?

Provided there aren't issues with Mac, it might be possible to use <clocale> here as a more portable means of referencing locale.h

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 22 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: trailing whitespace. (Sure there's other trailing whitspace in this file, but this is new to the patch.)

Done

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 108 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Perhaps I'm being obtuse. But I couldn't find documentation that suggests that the errno value provides greater insight than isfinite. Do you have a link? And if it does, might be worth including the link here.

Various docs for the C strtod* versions - cstdlib/strtod, Apple either say implementations "may" or will use errno to indicate range errors.

[SeanCurtis-TRI]

Reviewed 2 of 2 files at r4.
Reviewable status: 3 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @calderpg-tri)

---

geometry/render/gl_renderer/shape_meshes.cc, line 54 at r4 (raw file):

      tinyobj::LoadObj(&attrib, &shapes, &materials, &warn, &err, input_stream,
                       material_reader, do_tinyobj_triangulation);
  // TODO(SeanCurtis-TRI) Consider improving this exception message to clarify

BTW: This is not the TODO we need. See note below.

---

geometry/render/gl_renderer/shape_meshes.cc, line 142 at r4 (raw file):
nit: This is where the defect comes:

TODO(SeanCurtis-TRI) PR 14656 changed parse semantics. This error condition appears to no longer be reachable (it no longer appears in the unit tests) and the condition that this detects won't trigger this helpful message. Either clean up this case or find a way to give this feedback under the new tinyobj.

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 13 at r3 (raw file):

Previously, calderpg-tri wrote…

Provided there aren't issues with Mac, it might be possible to use <clocale> here as a more portable means of referencing locale.h

By definition, this can't affect the Mac because of the #ifdef fun and games. That said, I'm not going to care.

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 108 at r3 (raw file):

Previously, calderpg-tri wrote…

Various docs for the C strtod* versions - cstdlib/strtod, Apple either say implementations "may" or will use errno to indicate range errors.

I'd certainly seen the cplusplus link. In that case, its writing to errno is 100% correlated with returning a non-finite value.

In the case of the mac which attempts to signal underflow, the question is, do we care? We're getting zero as the result, and if someone is trying to provide a value that is smaller than the smallest normalized double....is treating that as zero a bad thing? If that is a bad thing, I think it should be clear that we're doing that in this patch. (And of course, where that line is probably depends on whether denormalized values are enabled or not.)

[calderpg-tri]

Reviewable status: 3 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @calderpg-tri and @SeanCurtis-TRI)

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 108 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'd certainly seen the cplusplus link. In that case, its writing to errno is 100% correlated with returning a non-finite value.

In the case of the mac which attempts to signal underflow, the question is, do we care? We're getting zero as the result, and if someone is trying to provide a value that is smaller than the smallest normalized double....is treating that as zero a bad thing? If that is a bad thing, I think it should be clear that we're doing that in this patch. (And of course, where that line is probably depends on whether denormalized values are enabled or not.)

My read is that pre-C++11, errno was set to ERANGE on underflow everywhere, and post C++11 implementations "may" set it. I'm fine with removing the errno handling and accepting zero in these cases, I don't think that breaks any expectations for our use of objs.

[calderpg-tri]

Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

---

geometry/render/gl_renderer/shape_meshes.cc, line 142 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This is where the defect comes:

TODO(SeanCurtis-TRI) PR 14656 changed parse semantics. This error condition appears to no longer be reachable (it no longer appears in the unit tests) and the condition that this detects won't trigger this helpful message. Either clean up this case or find a way to give this feedback under the new tinyobj.

Done

[jwnimmer-tri]

Reviewed 1 of 1 files at r5.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @calderpg-tri)

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 108 at r3 (raw file):

Previously, calderpg-tri wrote…

My read is that pre-C++11, errno was set to ERANGE on underflow everywhere, and post C++11 implementations "may" set it. I'm fine with removing the errno handling and accepting zero in these cases, I don't think that breaks any expectations for our use of objs.

My vote is for removing it.

[jwnimmer-tri]

Reviewed 1 of 1 files at r6.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @calderpg-tri)

[calderpg-tri]

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

tools/workspace/tinyobjloader/faster_float_parsing.patch, line 108 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

My vote is for removing it.

Done

[jwnimmer-tri]

Tested locally on macOS. Merging now.