This initial patch adds a MutableELFObject class for doing mutations on sections of an ELFObjectFile.
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?
protected? Does this class have any subclasses?
Use explicit here. Should this be an r-value reference? What do you think?
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.
What is this function for?
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.
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.
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.
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?
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.
You're right about Optional, sorry for the noise.
getSectionName currently calls getConstIfNew, so this function doesn't need to exist, right?
Does this need to exist? If so, it needs unit-testing.
Does this need to exist? If so, it needs unit-testing.
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.
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.
I'd rephrase the second half of this sentence as "methods work the same in both ELFObjectFile and MutableELFObject".
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;
This should be ASSERT_EQ, since the rest of the test assumes there's the same number.
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().
You need to assert somewhere that NumSections is as expected.
You need to show the iterator is still valid after this, by checking std::distance.
I'm pretty sure you don't need the UL suffix here. Same below.
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?
Probably this should be handleError(MutSecOrErr.takeError());
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
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?
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?
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.
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.
I have ASSERT_EQ(NumSections, 7); it ended up being 7 sections because of additional sections that yaml2obj added. Is this what you meant?
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.
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)
How about a comment for MutableELFObject?
"regardless if it" sounds off to me. How about "regardless of whether it"
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).
I have no idea which method this was referring to any more...
overrie -> override
wether -> whether
Sorry, I missed that moveSectionNext() is an override.
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.
Don't use auto here.
Yes, that's right.
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.
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.
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
Not really sure what to say, is that enough you think?
Same as above, not quite sure what to say.
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.
Is this what you mean? And then remove the one on 156? Sorry wasn't quite sure.
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.
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.
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.
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).
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.
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.
I'd change this slightly:
... an ELFObjectFile. The ELFObjectFile class only provides ...
You should be able to use the more common:
using Elf_Shdr = typename ELFT::Shdr;
"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.
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.