- User Since
- Jan 18 2017, 2:49 AM (166 w, 3 d)
I've added a few reviewers who are probably better placed to review things from the DWARF/DW_OP perspective than me.
Looks good from my perspective. I'll leave it up to @MaskRay to approve once you've taken a look at his suggestion.
Do you think a regular llvm-ar archive test is sufficient for this testing, or do you think a mach-o fat archive is important to test independently? I don't see a massive need for the latter personally.
LGTM, from a stylistic point of view. Not sure I can comment much on the behaviour parts that I haven't commented on. Please wait for other reviewers to approve too.
LGTM, with one fix.
LGTM. Please wait for @MaskRay to approve too, since I don't know very much about the details here.
Thu, Mar 26
Address review comments.
Remove unneeded test input.
Mention the static analyzer and other tools explicitly.
Looks fine to me. I'd like to see an example of it being used in a real test case, so I'm holding off on the LGTM until that point.
LGTM, once my last comment has been addressed and question answered (assuming that the question doesn't highlight an issue in the patch itself). Thanks for putting up with my lengthy review!
LGTM, with nit.
With all of our tools with GNU equivalents, we should avoid single letter aliases unless we can get GNU to buy into the new option too. Long names are less of an issue, as it's unlikely we'll end up with a clash.
I think this looks okay to me. LGTM. Caveat: I don't know much about the disassembler, so I leave it up to you whether you think somebody else should review.
Just to make sure I understand the issue: if DT_STRTAB is small enough to fit within a segment, according to the segment's size/offset fields, we currently crash, if the size/offset fields themselves are invalid? What about if only part of the DT_STRTAB range is within the file (i.e. DT_STRTAB is less than file size but DT_STRTAB + DT_STRSZ is past the end)?
Latest veresion basically looks fine. One suggestion and one question remaining from me.
I haven't really reviewed the funcional parts of this change in the attribute parser stuff, but everything else LGTM. Please wait for somebody else to review the attribute parser bits.
Wed, Mar 25
We might want to change the iterators to use the fallible iterator pattern described in the programmer's manual. That should ensure that all clients can cope with bad iteration somehow. I'm not sure I have any particular further insight into how to proceed here beyond that. I think imitating GNU's behaviour for now is a good start, and further improvements can be in later patches.
Code all looks fine now, just a few minor test suggestions.
Two minor comments. Otherwise, looks good to me.
Given @ikudrin requested changes earlier, he'll need to approve it for this patch to move to "Approved" state officially, not that it matters that much.
LGTM, with one minor suggestion. It might make sense to get one of the other reviewers formal approval before landing this.
Thanks for the effort. This is nearly there.
LGTM, with nit, with regards to the llvm-readobj and libObject parts. I haven't looked at the other bits though.
Tue, Mar 24
What, if any, are the actual behaviour changes in this patch? I don't see any test updates, but the patch isn't labelled NFC.
Mon, Mar 23
LGTM, with one minor suggestion.
@Quuxplusone - did you see that there are several clang-format warnings reported against these changes? Please could you fix them.
Fri, Mar 20
@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.
Make --implicit-check-not patterns tighter.
(Woops, one comment didn't get added yesterday).
Add colons to --implicit-check-not=warning (missed because they didn't appear in the diff, as they were in the old test).
Address review comments from @MaskRay.
LGTM, with a couple of nits.
Sorry, I've been quite busy with having about 15-20 different active reviews at once, some of which are not trivial in complexity. It may take me a couple of working days to respond to things.
Thu, Mar 19
Address review comments.
Address review comments.
I've created D76425 to remove the check in llvm-readobj for fully linked output. This would work as an alternative to this linker change, to fix the PR (although there is one minor caveat - see the bug comment/that review), but I don't think this linker change hurts still. The llvm-readobj patch solves the issue for the case where stack_sizes sections have all been concatenated together via linker script.
LGTM. @Higuoxing, would you mind writing a separate dedicated test for --dynamic? It would allow for testing the corner cases, which are outside the scope of this test file. It can be a separate patch, no problem.
Forgot to mention: given this is intended to be common across other file formats, and there is a need to test the isDebugSection functions, I'd recommend adding tests for COFF and/or Mach-O as well as the ELF one. They don't need to be comprehensive necessarily.
LGTM, but I'd like to see at least one other change that makes use of this to be ready before you commit this, as otherwise this is just dead and untested code. I agree a unit test probably doesn't make much sense.
LGTM, with comment updates.
I'm happy with this idea in principle, but does it work for users who have .stack_sizes grouped in linker scripts explicitly? (I'm betting not, and yes, such linker scripts do exist).
Wed, Mar 18
Fix unchecked Expected.