[Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1]

[abrachet]

Summary

This initial patch adds a MutableELFObject class for doing mutations on sections of an ELFObjectFile.

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

122

Do you need this to be a friend, or can you just pass in another argument or two (if needed) into the constructor of MutableELFSection?

128

protected? Does this class have any subclasses?

144

Use explicit here. Should this be an r-value reference? What do you think?

161–162

You're going to need to do something with the Error inside Expected here. This function should probably return an Expected too, so that you can pass that error up to a higher level to be handled.

169

What is this function for?

[abrachet]

Addressed review comments

[abrachet]

In D64281#1617212, @jhenderson wrote:

Since you're introducing a new class in a public header, you should write doxygen comments for at least the public/protected interface, and quite possibly any private bits too. I think if you are careful with those comments too, it will help explain the design of the classes.

I've not looked at the unit tests today. I'll do that tomorrow. The interface looks a lot cleaner than it did when I last looked at this. Hopefully adding comments will make it much clearer what's going on. If you make MutableTable a sub-class, then my concerns about the leaky implementation (i.e. needing to decide between fetching the section properties from the table or the base version) are significantly lessened.

I'm still working on the comments for the private interface although they are coming. I do have just the one for the one public method not inherited from ObjectFile.

llvm/include/llvm/Object/MutableELFObject.h

56

This was referencing getOriginal before.

In symbols for example, the top half of the DataRef is the section index of the symbol table. In that case I use getOriginal to see its sh_type. It wouldn't make sense to use the updated section list in this case I think.

71

This was referencing getIfNew previously.

It's for methods which could not use just the OrigType to do their job properly. For example, getSectionName returns the Name member when the section has been modified because there is no way to describe the change to ELFObjectFile through its sh_name field.

Optional does not work with references so far as I can tell. Is this correct? A copy would be too costly for MutableELFSections which have an OwningArrayRef and std::string.

144

Sounds fine to me. In the unit tests I move the ELFObjectFile but then continue to use it because as @rupprecht pointed out previously the move constructor is just a copy constructor. Is it safe to do this or should I explicitly copy?

169

It advances the section_iterator. MutableELFObject uses a different iterator to that of ELFObjectFile which I belive would have a moveSectionNext that did something like Sec.p += sizeof(Elf_Shdr) as those just point directly to the section header.

[abrachet]

Realized from the code example in doxygen comment that it was very unergonomic, added a better overload to public methods which take a SectionRef so they can be used in for-each loops.

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

71

You're right about Optional, sorry for the noise.

getSectionName currently calls getConstIfNew, so this function doesn't need to exist, right?

118

Does this need to exist? If so, it needs unit-testing.

122

Does this need to exist? If so, it needs unit-testing.

144

Is it safe to do this or should I explicitly copy?

This doesn't sound safe to me - you need to copy or just simply not use the object after it's been moved. a) I wouldn't be surprised if this is technically undefined behaviour and the compiler makes assumptions about the state of the instance after the move. b) It's fragile to future changes.

169

At the moment, it doesn't look like it's used, so it shouldn't exist. Re-add it when it is needed. However, perhaps a safer thing to do is define your own iterator class, as @jakehehrlich said that does this internally.

llvm/unittests/Object/MutableELFObjectTest.cpp

22–23

I'd rephrase the second half of this sentence as "methods work the same in both ELFObjectFile and MutableELFObject".

44

Isn't the solution here to just make a second local variable that's a pointer to an ELFObjectFile constructed from MutableObject?

ELFObjectFile *ElfObject = &MutableObject;

47

ObjFileSecCount?

49

MutObjSecCount?

51

This should be ASSERT_EQ, since the rest of the test assumes there's the same number.

79

IIRC, this will still abort if there is an error, since the error itself hasn't been handled. The correct thing to do would be to a) check there's no Error, and then b) call consumeError().

115

You need to assert somewhere that NumSections is as expected.

126

You need to show the iterator is still valid after this, by checking std::distance.

193

I'm pretty sure you don't need the UL suffix here. Same below.

[jhenderson]

Aargh, you pushed your latest update whilst I was doing my last review, so I can't see what are the latest changes!

added a better overload to public methods which take a SectionRef so they can be used in for-each loops.

What is this bit referring to?

llvm/include/llvm/Object/MutableELFObject.h

160

Probably this should be handleError(MutSecOrErr.takeError());

[abrachet]

Addressed review comments

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

118

Yes it is used and thus tested implicitly. How would I go about testing the private interface? I could make it protected and then inherit from it in the unit test, but that doesn't sound great. FWIW this is almost identical to the getHeader method in ELFFile. https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Object/ELF.h#L108

122

Yes. It is used for every public method on SectionRef that gets tested in the unit test. Is this enough or should I test it directly as well?

144

I did an explicit copy /I think/ in the unit test now. I have an ELFObjectFile * and then deference it and assign to an ObjectFile &. Does this make a copy or does the compiler omit it and just assign the address?

169

This was referring to moveSectionNext() before.

It does get used, it is an overload of the function that content_iterator::operator ++() calls. It must be overriden because ELFObjectFile::moveSectionNext() iterates over sections not with their index but their mapped address. This uses index into MutableTable.

llvm/unittests/Object/MutableELFObjectTest.cpp

44

I don't think so. Then both will be MutableELFObject's and the same virtual methods will be called for each, I believe. I think testing the methods on an ELFObjectFile and MutableELFObject test that overloads for MutableELFObject behave the same as the methods in ELFObjectFile when the sections haven't been changed.

79

Thanks!

115

I have ASSERT_EQ(NumSections, 7); it ended up being 7 sections because of additional sections that yaml2obj added. Is this what you meant?

193

I don't actually need the L it turns out. Without making it unsigned I get this warning comparison of integers of different signs on clang.

[abrachet]

In D64281#1619189, @jhenderson wrote:

Aargh, you pushed your latest update whilst I was doing my last review, so I can't see what are the latest changes!

added a better overload to public methods which take a SectionRef so they can be used in for-each loops.

What is this bit referring to?

I added getMutableSection(SectionRef) and changed getMutableSection(section_iterator Sec) to just call that with a *Sec. Maybe I should remove the section_iterator overload though?

[labath]

llvm/include/llvm/Object/MutableELFObject.h

144

Kind of the whole point of references is that they allow you to "refer" to other objects without making copies of them. If you want to make a copy, drop the & and assign it to an ObjectFile (or ELFObjectFile, or something)

[abrachet]

Create a new ObjectFile in unit test

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

144

Oh yeah. What am I talking about!

[abrachet]

Added doxygen comments to private interface. NFC

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

21

Comment?

44

How about a comment for MutableELFObject?

87

"regardless if it" sounds off to me. How about "regardless of whether it"

118

Sorry, missed that getHeader was private.

Being able to modify the header contents is a required thing however, so you might want to consider how this could be done (possibly a different change though).

122

I have no idea which method this was referring to any more...

141

overrie -> override

142

wether -> whether

169

Sorry, I missed that moveSectionNext() is an override.

llvm/unittests/Object/MutableELFObjectTest.cpp

55

Rather than reusing the same variable, create a new variable here. Then don't bother with the ObjFile variable above. It doesn't give us anything.

89

Don't use auto here.

115

Yes, that's right.

142

EXPECT_EQ -> ASSERT_EQ to avoid dereferencing an invalid iterator later on.

I think I either misread the code last time, as I don't know why I wanted this check here. However, you do show that the std::distance from section_begin to section_end is still correct after you called getMutableSection, but not until after you've done the iteration, meaning that your iterator could be invalid part way through your calls to compareSectionName. Move the later std::distance check up to here instead.

193

Sounds like clang isn't doing a good enough job here - it's a spurious warning, because 2 is a positive integer literal so can clearly be handled properly in this comparison.

Check elsewhere, but I thought it was more common in the code base to use lower-case for literal suffixes when they're needed.

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

122

Oops I think I responded to this before adding the comments so I forgot to add a what the comment was originally referring to.

It was getSection(DataRefImpl). Its the function that I made virtual from ELFObjectFile. All the tests that I have now like FirstSec->getSize() test this method

[abrachet]

Addressed review comments

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

21

Not really sure what to say, is that enough you think?

44

Same as above, not quite sure what to say.

llvm/unittests/Object/MutableELFObjectTest.cpp

55

I think that it does give something, The ObjectFiles are made from the same yaml so I'm testing that the public methods return the same values. Its a whole thing because ObjectFile has no copy constructor. Also we don't want to use the ObjectFile after moving it. I also still think that doing something like this

MutableELFObject<ELF64LE> MutableObject(std::move(*MutELFObjFile));
ELFObjectFile<ELF64LE> *ElfObject = &MutableObject;

// test the interface

will never produce any differences so it doesn't test that MutableELFObject has the same behavior as ELFObjectFile on an unmodified object file.

142

Is this what you mean? And then remove the one on 156? Sorry wasn't quite sure.

193

FWIW I think there is there's a function template instantiation somewhere in these macros and so its doing unsigned_expr == signed_expr not unsigned_expr == 2.

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

21

Why would you use this class? What is the overall aim here?

It probably wants to say something like it is used by MutableELFObject to provide a mutable version of an ELFObjectFile Section or something to that effect.

44

I think this comment needs to explain why this class exists. Essentially it should say why we can't use ELFObjectFile directly for our purposes.

llvm/unittests/Object/MutableELFObjectTest.cpp

55

I think you misunderstood. Having MutELFObjFile is fine, as is constructing two objects from the YAML. ObjFile doesn't need to exist however. You can use ELFObjFile directly now that you're not reusing the local variable for the creation of MutableObject, so the two variables you operate on are ELFObjFile (created directly from YAML) and MutableObject (created indirectly from YAML via MutELFObjFile).

142

Sorry, I obviously wasn't clear enough, and I realised something else whilst writing this comment.

At the time of writing, the std::distance at line 127 (checks the section count before mutating), and the one at line 142 (checks it after mutating) are the ones that should exist. The one at line 155 is unnecessary, since you check it at line 142. The one at line 140 is necessary because it shows that the Iter variable is still valid after getting a mutable section.

[abrachet]

Addressed review comments

[jhenderson]

Aside from a comment change, this looks good to me. I'd wait for others to approve too though, and I wouldn't necessarily land it even then until some later patches are ready too, although I'm not sure about that.

llvm/include/llvm/Object/MutableELFObject.h

46

I'd change this slightly:

... an ELFObjectFile. The ELFObjectFile class only provides ...

[rupprecht]

llvm/include/llvm/Object/MutableELFObject.h

24

You should be able to use the more common:

using Elf_Shdr = typename ELFT::Shdr;

82–85

"OriginalValues" is overloaded, so it's not clear which one is being referenced in the constructor

A common hacky practice in LLVM is to name the constructor arg "TheOriginalValues" or something -- that's fine to use if you can't come up with any other different name.

135–136

ditto

llvm/unittests/Object/MutableELFObjectTest.cpp

89–90

Is it expected that an error here should not cause the test to fail? I think you want an assertion that Expect doesn't have an error instead.

[abrachet]

Addressed review comments

[jhenderson]

Latest changes LGTM.