This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Add --prefix-alloc-sections
ClosedPublic

Authored by seiya on Mar 31 2019, 12:57 AM.

Diff Detail

Event Timeline

seiya created this revision.Mar 31 2019, 12:57 AM
rupprecht added inline comments.Apr 1 2019, 10:16 AM
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
16

section content seems irrelevant to this test, so I think you can just remove content/--section-data

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
596

Typical llvm style is to only include braces if there is more than one statement, i.e. this should just be:

if (!Config.AllocSectionsPrefix.empty())
  for (auto &Sec : Obj.sections())
    if ((Sec.Flags & SHF_ALLOC) != 0)
      Sec.Name = (Config.AllocSectionsPrefix + Sec.Name).str();
seiya updated this revision to Diff 193189.EditedApr 1 2019, 4:00 PM
  • Removed section contents from tests.
  • Removed braces.
rupprecht accepted this revision.Apr 1 2019, 10:35 PM

Looks good to me, thanks! Do you need one of us to commit on your behalf?

This revision is now accepted and ready to land.Apr 1 2019, 10:35 PM
seiya added a comment.Apr 2 2019, 2:13 AM

Thank you for taking your time!

Looks good to me, thanks! Do you need one of us to commit on your behalf?

Yes please.

Thanks for the patch. Before this goes in, I'd like to ask one question:

Does GNU objcopy do anything with relocation sections to sections with changed names. I suspect that it renames them to match, but I don't think this patch does that.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
179–180

Nit: It would probably make sense for this to swap with Config.AddSection.empty() to match the member order.

llvm/tools/llvm-objcopy/CopyConfig.cpp
505

Nit: this line hasn't been clang-formatted.

seiya added a comment.Apr 2 2019, 9:41 PM

Thanks for the patch. Before this goes in, I'd like to ask one question:

Does GNU objcopy do anything with relocation sections to sections with changed names. I suspect that it renames them to match, but I don't think this patch does that.

It seems so:

$ echo "int x; int foo() { return x; }" > test.c
$ clang -c test.c -o test.o
$ objcopy --prefix-alloc-sections=.alloc_prefix test.o
$ llvm-readobj --sections test.o | grep Name
LoadName:
    Name:  (0)
    Name: .alloc_prefix.text (32)
    Name: .rela.alloc_prefix.text (27)
    Name: .comment (51)
    Name: .note.GNU-stack (60)
    Name: .alloc_prefix.eh_frame (81)
    Name: .rela.alloc_prefix.eh_frame (76)
    Name: .symtab (1)
    Name: .strtab (9)
    Name: .shstrtab (17)

I'll update the patch to handle this.

seiya updated this revision to Diff 193432.Apr 2 2019, 9:50 PM
  • Rename relocation section names associated to the allocated sections.
seiya marked 4 inline comments as done.Apr 2 2019, 9:53 PM
jakehehrlich requested changes to this revision.Apr 2 2019, 11:00 PM

I'm not sure I like renaming and I'm not sure anyone requires that behavior. It also introduces n^2 behavior which I don't like. That should be removed at the very least.

Also don't we already have an update sections loop somewhere in this already? I think this should go there to reduce the number of passes we make over the sections

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
596

Do we follow that even for such deep nestings? I think I've only been doing it 2 layers deep before I add brackets to the outer most block.

This revision now requires changes to proceed.Apr 2 2019, 11:00 PM

I'm not sure I like renaming and I'm not sure anyone requires that behavior.

From the bug in the patch description, this is used by linux here: https://github.com/ClangBuiltLinux/linux/blob/master/drivers/firmware/efi/libstub/Makefile#L75

(I don't have time to review the latest patch tonight, but I can take a look tomorrow)

Ah well then we should implement that behavior. We should get rid of the n^2 loop however.

Also don't we already have an update sections loop somewhere in this already? I think this should go there to reduce the number of passes we make over the sections

It looks like we already have multiple loops over the sections so that some flags can apply on top of each other, though it's possible a few of the passes could be combined. O(n^2) is really the only real performance killer though, simply iterating over sections multiple times is a *much* less bigger deal.

Quick check:

grep -c ": Obj.sections())" llvm/tools/llvm-objcopy/ELF/* | grep -v :0
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:6
llvm/tools/llvm-objcopy/ELF/Object.cpp:11
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
4

It'd be good to check --relocs as well

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
602

Instead of the O(n^2) loop over the sections, you can do it one pass:

  • If Name == Config.AllocSectionsPrefix then do the usual rename
  • If is_a<RelocationSectionBase>(&Sec) and dyn_cast<RelocationSectionbase>(&Sec)->getSection()->getName() == Config.AllocSectionsPrefix, then change based on the .rel/.rela pattern below

getSection() is probably not well named since that makes it sound like a regular getter, it should probably be getTargetSection() or similar.

seiya updated this revision to Diff 193641.Apr 3 2019, 6:37 PM
  • Rename sections in one pass.
  • Check --relocs in a test.
seiya marked 2 inline comments as done.Apr 3 2019, 6:37 PM
jhenderson added inline comments.Apr 4 2019, 1:36 AM
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
4

Dumb question maybe, but what does this give us?

llvm/test/tools/llvm-objcopy/ELF/rename-section-and-prefix-alloc-sections.test
18

Not that it really matters, but you don't need the SHF_EXECINSTR here, as it has no impact on the behaviour.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
598

Hmm... What does GNU objcopy do for dynamic relocation sections (i.e. SHT_REL[A] with SHF_ALLOC)? Will it rename them according to the switch, or something else (e.g. renaming based on their target section). In other words, if for example I had a .rela.plt section targeting .plt and I did --prefix-alloc-sections=.foo, do you get .foo.rela.plt or .rela.foo.plt or something else?

You need a test case for this too.

seiya marked an inline comment as done.Apr 4 2019, 3:37 AM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
598

It seems that GNU objcopy does not support such sections [1]. It produces .foo.rela.plt but does not preserve its link field:

$ readelf -S hello
There are 29 section headers, starting at offset 0x19e0:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .prefix.interp    PROGBITS         0000000000000238  00000238
       000000000000001c  0000000000000000   A       0     0     1
  [ 2] .prefix.note.ABI- NOTE             0000000000000254  00000254
       0000000000000020  0000000000000000   A       0     0     4
  [ 3] .prefix.note.gnu. NOTE             0000000000000274  00000274
       0000000000000024  0000000000000000   A       0     0     4
  [ 4] .prefix.gnu.hash  GNU_HASH         0000000000000298  00000298
       000000000000001c  0000000000000000   A       5     0     8
readelf: Warning: [ 5]: Link field (0) should index a string section.
  [ 5] .prefix.dynsym    DYNSYM           00000000000002b8  000002b8
       00000000000000a8  0000000000000018   A       0     1     8
  [ 6] .prefix.dynstr    STRTAB           0000000000000360  00000360
       0000000000000082  0000000000000000   A       0     0     1
  [ 7] .prefix.gnu.versi VERSYM           00000000000003e2  000003e2
       000000000000000e  0000000000000002   A       5     0     2
  [ 8] .prefix.gnu.versi VERNEED          00000000000003f0  000003f0
       0000000000000020  0000000000000000   A       6     1     8
readelf: Warning: [ 9]: Link field (0) should index a symtab section.
  [ 9] .prefix.rela.dyn  RELA             0000000000000410  00000410
       00000000000000c0  0000000000000018   A       0     0     8
readelf: Warning: [10]: Link field (0) should index a symtab section.
  [10] .prefix.rela.plt  RELA             00000000000004d0  000004d0
       0000000000000018  0000000000000018   A       0     0     8
  [11] .prefix.init      PROGBITS         00000000000004e8  000004e8
       0000000000000017  0000000000000000  AX       0     0     4
  [12] .prefix.plt       PROGBITS         0000000000000500  00000500
       0000000000000020  0000000000000010  AX       0     0     16
  [13] .prefix.plt.got   PROGBITS         0000000000000520  00000520
       0000000000000008  0000000000000008  AX       0     0     8
  [14] .prefix.text      PROGBITS         0000000000000530  00000530
       00000000000001a2  0000000000000000  AX       0     0     16
  [15] .prefix.fini      PROGBITS         00000000000006d4  000006d4
       0000000000000009  0000000000000000  AX       0     0     4
  [16] .prefix.rodata    PROGBITS         00000000000006e0  000006e0
       000000000000000a  0000000000000000   A       0     0     4
  [17] .prefix.eh_frame_ PROGBITS         00000000000006ec  000006ec
       000000000000003c  0000000000000000   A       0     0     4
  [18] .prefix.eh_frame  PROGBITS         0000000000000728  00000728
       0000000000000108  0000000000000000   A       0     0     8
  [19] .prefix.init_arra INIT_ARRAY       0000000000200db8  00000db8
       0000000000000008  0000000000000008  WA       0     0     8
  [20] .prefix.fini_arra FINI_ARRAY       0000000000200dc0  00000dc0
       0000000000000008  0000000000000008  WA       0     0     8
readelf: Warning: [21]: Link field (0) should index a string section.

I'll add a test case for this.

[1]: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/elf.c;h=952e4bfe11b4a222823a31d07615007e91b519ba;hb=HEAD#l2398

seiya updated this revision to Diff 193688.EditedApr 4 2019, 4:34 AM
  • Updated a test case for dynamic relocation sections (SHT_REL[A] with SHF_ALLOC).
    • This makes sure that .rela.plt is renamed to .prefix.rela.plt, not .rela.prefix.plt (GNU objcopy does so).
  • Removed an unnecessary flag from a test.
jhenderson accepted this revision.Apr 4 2019, 4:45 AM

LGTM, but please wait for one of the other reviewers too.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
603

Probably worth a comment saying that this doesn't apply to dynamic relocation sections and why.

jhenderson added inline comments.Apr 4 2019, 4:45 AM
llvm/tools/llvm-objcopy/ObjcopyOpts.td
226

Nit: names -> name

seiya updated this revision to Diff 193694.Apr 4 2019, 5:34 AM
seiya marked an inline comment as done and an inline comment as not done.
  • Added a comment which describes how it handles dynamic relocations sections and why.
  • Fixed a help message.
seiya marked 4 inline comments as done.Apr 4 2019, 5:34 AM
seiya updated this revision to Diff 193695.Apr 4 2019, 5:37 AM
  • Removed a redundant whitespace from a comment.
jhenderson added inline comments.Apr 4 2019, 5:38 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
606

Nit: remove the "in".

Just a couple small comments, but also LGTM

llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
60

Checking the } isn't necessary (unless you're checking a whole section w/ CHECK-NEXT)

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
179–181

I think this block needs to be clang formatted

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
623–627

Can you add a comment why this block is necessary?

It looks like all the test coverage is .rel sections coming after their target section. Is it possible to add a test case for the other half of this clause? e.g. a section w/ .rel.data coming before .data?

seiya updated this revision to Diff 193816.EditedApr 4 2019, 6:08 PM
  • Updated a test case to test that it renames properly when a relocation section comes before its target section.
  • Removed unnecessary } from a test.
  • Added a comment which describes the case when a relocation section comes before its target section.
  • Fixed a comment.
  • Applied clang-format.
seiya marked 4 inline comments as done.Apr 4 2019, 6:09 PM
jhenderson added inline comments.Apr 5 2019, 1:12 AM
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
4

I'm still not sure what extra coverage --relocs gives us. Could somebody explain, please?

seiya marked an inline comment as done.Apr 6 2019, 6:59 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
4

I think it would be good to make sure that this feature (--prefix-alloc-sections) does not destroy the contents of relocation sections other than their names. I should add a comment here.

How do you think @rupprecht ?

rupprecht accepted this revision.Apr 9 2019, 11:22 AM

I'm out the rest of the week, I'll leave it to James to do the commit on your behalf

llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
4

After talking to James offline, the --relocs effectively isn't testing anything in addition to what's already covered, so it should be fine to remove. Sorry for suggesting that.

seiya updated this revision to Diff 194424.Apr 9 2019, 5:35 PM
  • Removed --relocs from a test.
seiya marked 4 inline comments as done.Apr 9 2019, 5:35 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
4

No problem! I've updated the diff.

seiya marked an inline comment as done.Apr 9 2019, 5:37 PM

Having looked at this a bit more, I wonder if this should be part of the existing SectionsToRename loop. After all, there isn't much difference between the two behaviours, apart from the fact that this new switch doesn't change the flags. Would you mind folding the two loops together? As a side effect of this, I expect you might want to rename relocation sections when --rename-section alone is used, which I don't think is currently happening.

Also @jakehehrlich explicitly requested changes, I'd like him to give a thumbs up before I commit this. Thanks for the work @seiya!

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
555–556

This is the loop I'm talking about in the out-of-line comment.

seiya added a comment.Apr 11 2019, 6:02 AM

Having looked at this a bit more, I wonder if this should be part of the existing SectionsToRename loop. After all, there isn't much difference between the two behaviours, apart from the fact that this new switch doesn't change the flags. Would you mind folding the two loops together? As a side effect of this, I expect you might want to rename relocation sections when --rename-section alone is used, which I don't think is currently happening.

That's a good idea. I'll fold them together.

seiya updated this revision to Diff 194668.EditedApr 11 2019, 6:06 AM
  • Merged the existing SectionsToRename loop and the AllocSectionsPrefix loop into a single loop.
seiya marked an inline comment as done.Apr 11 2019, 6:06 AM
jhenderson added inline comments.Apr 11 2019, 6:13 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
555

You should probably keep this if, but expand it to be:
if (!Config.SectionsToRename.empty() || !Config.AllocSectionsPrefix.empty())

That will mean that this loop only happens if one or both of the options is specified.

572

Nit config -> Config to match the variable name.

seiya updated this revision to Diff 194671.Apr 11 2019, 6:28 AM
  • Check if one or both of --rename-section/--prefix-alloc-sections options is specified.
  • Fixed a comment.
seiya marked 2 inline comments as done.Apr 11 2019, 6:29 AM
jhenderson accepted this revision.Apr 11 2019, 6:33 AM

LGTM. As I mentioned earlier, @jakehehrlich should review this before it gets committed. However, he may be away, so you might have to wait a few days.

rupprecht accepted this revision.Apr 15 2019, 1:31 AM

A few comments.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
580

You can do if (auto *RelocSec = dyn_cast<RelocationSectionBase>(&Sec)) and then SectionBase *TargetSec = RelocSec->getSection()

580

I forget. Does this apply to dynamic relocation sections as well? Does this flag rename the dynamic relocation section? My hunch is that it doesn't.

599

This is not a good way to do this.

I think the right way is to have the "OriginalName" and do this in a non-conditional way. I tried thinking about a bunch of alternatives but they all have sticky issues. Your best bet is to just add OriginalName to SectionBase and unconditionally assign the section name here.

seiya marked 2 inline comments as done.Apr 17 2019, 9:42 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
580

Yes. This flag renames dynamic relocation sections too. It seems that GNU objcopy simply checks whether ALLOC flag is set:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=binutils/objcopy.c;h=330b93c1ad3f87a0f5777ac88dc4af1398959bbe;hb=HEAD#l3772

599

I think that's not sufficient. If both --rename-section and --prefix-alloc-sections are given, GNU objcopy renames sections by --rename-section first and then adds the prefix specified by --prefix-alloc-sections.

For example, --rename-section=.text=.text2 --prefix-alloc-sections=.prefix renames .rel.text to .rel.prefix.text2.

It looks that the current patch only renames correctly if the target section comes before its relocation section by the way.

I have two ideas:

  • Iterate sections twice: rename sections first and then add the prefix.
  • Add Renamed flag to SectionBase: check the flag here and rename the section if needed.

I would like to hear your thoughts on this @jakehehrlich .

seiya marked an inline comment as done.Apr 19 2019, 5:11 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
599

Or add OriginalName to SectionBase and use Config.SectionsToRename here:

const auto Iter = Config.SectionsToRename.find(TargetSec->OriginalName);
if (Iter != Config.SectionsToRename.end()) {
  // Both `--rename-section` and `--prefix-alloc-sections` are given.
  const SectionRename &SR = Iter->second;
  Sec.Name = (prefix + Config.AllocSectionsPrefix + SR.NewName).str();
} else {
  Sec.Name = (prefix + Config.AllocSectionsPrefix + TargetSec->Name).str();
}
jakehehrlich added inline comments.Apr 22 2019, 12:20 PM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
580

Can you add a test case for this. We have a binary checked in that you can use. I want to make sure it isn't double renamed and all that. It's just a slightly more tricky real world use case.

599

Ah right, good catch. I think the proposal you just gave is better than what we currently have but the issue of the section being renamed should probably happen more smoothly I suppose. In principal we could add a new mechanism for section renaming in the future which would invalidate this and then require us to update it in the future.

What if you build a map of renamed sections here and then check if the section is renamed. That should avoid the fragility of checking for the prefix like this.

seiya updated this revision to Diff 196167.Apr 22 2019, 7:00 PM
  • Use dyn_cast
  • Add PrefixedSections to check whether the target section is already prefixed AllocSectionsPrefix.
  • Handle the case when --rename-section and --prefix-alloc-sections are given and the relocation section comes before its target section.
seiya marked 5 inline comments as done.Apr 22 2019, 7:06 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
55

This .rela.plt is a testcase what I mean.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
580

We have a test case which renames dynamic relocation sections. Should I clarify that in a comment or add a test case against a binary file, say, test/tools/llvm-objcopy/ELF/Inputs/dynrel.elf?

jakehehrlich added inline comments.Apr 25 2019, 2:17 PM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
580

Those two cases are slightly different and reflects more realistic set up than the yaml file does. Should be a short 5 or so line test.

600

I'm *really* against doing this kind of string searching when we can build a map. What if a section already contains the prefix being used. This won't be consistent in that case.

Also you should add a test case for that above.

seiya marked an inline comment as done.Apr 28 2019, 8:01 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
600

Let me clarify my understanding a bit. You mean a map which contains a pointer to a section like DenseSet<SectionBase *> PrefixedSections?

jakehehrlich accepted this revision.Apr 30 2019, 3:54 PM

LGTM

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
600

Ah whoops. I saw "find" and thought it hadn't changed. Big mistake on my part. Sorry. This LGTM.

This revision is now accepted and ready to land.Apr 30 2019, 3:54 PM
seiya marked an inline comment as done.Apr 30 2019, 6:27 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
600

Thank you for your comment. I'll update the patch.

seiya updated this revision to Diff 197516.May 1 2019, 2:36 AM
seiya marked 6 inline comments as done.
  • Add a test case: a section which already has the prefix specified by this option (.prefix.already_prefixed).
  • Use ELF/Inputs/dynrel.elf in a test instead of a binary constructed by yaml2obj.
  • Store a pointer to a SectionBase in PrefixedSections instead of its name.
seiya updated this revision to Diff 197518.May 1 2019, 2:49 AM
  • Rename a test file.
jhenderson added inline comments.May 1 2019, 2:51 AM
llvm/test/tools/llvm-objcopy/ELF/rename-section-and-prefix-alloc-sections.test
2

In this test, you don't really want to test the cases that you already tested in the main --prefix-alloc-sections test. As a result, you can probably remove the bar section, at the very least, nor do you need to test a section that isn't renamed at all.

I'm also not sure you get anything by testing two different section renames.

seiya marked 2 inline comments as done.May 1 2019, 7:35 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/ELF/rename-section-and-prefix-alloc-sections.test
2

Indeed. I'll remove the test.

I'm also not sure you get anything by testing two different section renames.

We should test the cases when the relocation section comes after/before its target section because this patch deals with them differently to perform both --rename-section and --prefix-alloc-sections in one pass.

I'll update the patch to mention it in this test file.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
604

I mean this if block.

seiya updated this revision to Diff 197694.May 1 2019, 7:40 PM
  • Add a comment in rename-section-and-prefix-alloc-sections.test.
  • Remove some test cases which already tested in another test.
jhenderson accepted this revision.May 2 2019, 1:26 AM

Great! LGTM. Would you like me to commit it?

MaskRay added inline comments.May 2 2019, 1:34 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
570

It looks most other places don't put != 0

MaskRay added inline comments.May 2 2019, 1:37 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
600

PrefixedSections.count(TargetSec)

jhenderson added inline comments.May 2 2019, 1:40 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
570

Hmm... I find != 0 easier to read personally, but I don't feel strongly about it.

seiya updated this revision to Diff 198127.May 3 2019, 10:52 PM
seiya marked 4 inline comments as done.May 3 2019, 10:59 PM
seiya added a comment.May 3 2019, 11:04 PM

Great! LGTM. Would you like me to commit it?

Yes please if it got accepted by others :)

Great! LGTM. Would you like me to commit it?

Yes please if it got accepted by others :)

Okay. As @MaskRay had some previous comments, I'll give him a chance to review further before committing.

MaskRay accepted this revision.May 7 2019, 4:22 AM
MaskRay added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
608

How about Sec.Name = (prefix + Config.AllocSectionsPrefix + Iter->second.NewName).str();

seiya updated this revision to Diff 198596.May 8 2019, 2:02 AM
seiya marked an inline comment as done.
  • Cleaned up the code a bit (suggested by @MaskRay). NFC.
seiya marked an inline comment as done.May 8 2019, 2:02 AM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
608

Looks better. Thanks.

seiya marked an inline comment as done.May 8 2019, 2:03 AM

Great! I'll get this landed later today.

This revision was automatically updated to reflect the committed changes.