[PATCH] D75742: [Object] Support large archive members

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

In D75742#1912140 <https://reviews.llvm.org/D75742#1912140>, @jhenderson wrote:

> Simplify size handling slightly.
>
> I've not deleted the test yet, but I will do so in due course before this gets committed. I'd like to see if we can come up with an alternative test option first, if we can. For the record, the test passes locally with my patch, but didn't without the fix (it was how I found the bug in the first place).


I don't know if it's portable (you'd have to add requires for Linux/shell, or maybe do some lit configuration to detect if it's available), but you could try using `fallocate`:

  $ time fallocate -l 3G test.large
  fallocate -l 3G test.large  0.00s user 0.01s system 39% cpu 0.017 total
  $ time cat test.large > /dev/null
  cat test.large > /dev/null  0.01s user 1.18s system 99% cpu 1.193 total

If you want to try that, I would still recommend keeping the unit test that runs on all platforms, and just having this lit one be an extra test.



================
Comment at: llvm/unittests/Object/ArchiveTest.cpp:66
+  auto Child = (*A)->child_begin(ChildErr);
+  ASSERT_THAT_ERROR(std::move(ChildErr), Succeeded());
+
----------------
jhenderson wrote:
> grimar wrote:
> > Not strong suggestion:
> > 
> > ```
> >   MemoryBufferRef Source(ArchiveWithMember, "regular");
> >   Expected<std::unique_ptr<Archive>> A = Archive::create(Source);
> >   ASSERT_THAT_EXPECTED(A, Succeeded());
> >   Error ChildErr = Error::success();
> >   auto Child = (*A)->child_begin(ChildErr);
> >   ASSERT_THAT_ERROR(std::move(ChildErr), Succeeded());
> > ```
> > 
> > This part is almost similar to one in `ArchiveChildGetSizeThinArchive` and `ArchiveChildGetSizeRegularArchive`.
> > Should this code be moved to a helper that returns a `Child`?
> I considered that. Unfortunately, ASSERT_* macros return void, so can't be inside other functions that return something else. Consequently, we have to put them in yet another function, then check to see if the thing is valid anyway (rendering the ASSERT pointless), and the logic becomes somewhat complex and hard to follow. I don't think there's much benefit as a result.
One way you might be able to refactor this is to not have the assert in the helper method:

```
Expected<Child> createChild(StringRef src, StringRef name) {
  MemoryBufferRef Source(src, name);
  Expected<std::unique_ptr<Archive>> A = Archive::create(Source);
  if (!A) return A.takeError();

  Error ChildErr = Error::success();
  auto Child = (*A)->child_begin(ChildErr);
  if (ChildErr) return std::move(ChildErr);
  return std::move(Child);
}

TEST(ArchiveTests, ArchiveChildGetBuffer) {
  auto Child = createChild(ArchiveWithMember, "regular");
  ASSERT_THAT_EXPECTED(Child, Succeeded());
...
```

(heavy pseudocode, I almost definitely got a lot of types wrong... and I'm also ignoring the lifetime issues)


================
Comment at: llvm/unittests/Object/ArchiveTest.cpp:71
+  // StringRef (which has an invalid size).
+  ASSERT_TRUE(Buffer.operator bool());
+  EXPECT_EQ(9999999999, Buffer->size());
----------------
`ASSERT_TRUE((bool)Buffer);`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75742/new/

https://reviews.llvm.org/D75742