- User Since
- Jan 18 2017, 2:49 AM (230 w, 5 d)
The rest of the patches in this series have been approved. It's just this patch that needs looking at. There's no particular need to look at most of the files that were in the debuginfo-test folder, as they've simply been moved. I'd recommend reviewing the changes outside that folder, plus the CMake files.
Please add the new option to the llvm-readobj documentation.
Fri, Jun 18
Not had time to review the test cases yet, but will do that next week.
Thu, Jun 17
This isn't an issue for the static linking approach, so why does it happen for the dynamic library approach? It seems like fixing this aspect should be the way forward, as it will be more robust generally.
Okay, no more comments from me at this time. Someone with GOFF knowledge needs to review the functionality for correctness.
Wed, Jun 16
I take it the new output is just what the head of LLVM produces when rerunning the examples? If so, LGTM.
Are call graph sections expected in fully linked output?
In other places, we've tested large obejcts by using a compressed input, and then extracted it at test time, although the examples I see are only a few megabytes in size, rather than gigabytes. I'm not convinced we want such such a large object lying around, so looks good to me.
Tue, Jun 15
LGTM too. Please remember to update the test once the string table printing has landed.
Mon, Jun 14
Just trying to follow the logic through. I think there is a potential behaviour change, which may be fine, but I just want to highlight it to see what others think (I have no use for IHEX myself, so don't feel like I can make a completely fair judgment either way). If I follow it correctly, if a writable section were not in a PT_LOAD segment, then previously it was skipped, if there was at least one writable section that was in such a segment, whereas now it won't be skipped. This is probably harmless (all such sections should have been placed in a segment anyway). Assuming I'm right, should we have a test-case to illustrate this behaviour change?
Fri, Jun 11
Maybe -C/-A/-B are available to match grep because the current option is quite long?
Fix missed clang-format issue, and unchecked Exception (caused by broken check).
LGTM, with nit.
The size went up from 107KB to 322KB, aggregate of all the input sections.
Thu, Jun 10
Wed, Jun 9
Tue, Jun 8
Lots of stylistic comments from me. Someone else will need to review functionality.
Mon, Jun 7
Test suggestion looks reasonable to me, although I might suggest one slight tweak, namely to use --implicit-check-not rather than CEHCK-NOT after the symbol check. In the latter case, there's nothing stopping the symbol appearing before the set that you are checking.
LGTM. It looks like there are non-standard line endings in this file - please fix them up whislt your're at it (either in the same or another commit).
Mostly looks good to me, but would be nice if one of the earlier reviewers gives thumbs up before we commit this patch.
Could you please split this patch into a patch that adds the new enum values, which would include the llvm-readobj and yaml2obj/obj2yaml changes, and then another patch (or patches) with the rest in? The latter builds on the former, but the former doesn't need the latter to work.
Looks good again. Sorry for the delay - I was off work last week.
Sorry for the delay - I was off work last week. Some minor comments, otherwise basically this is good.
Sat, May 29
Fri, May 28
@kwk, could you try again please with the latest version of the patch? The code now looks at the same paths as are added to the PATH environment variable, so it should work.
Use the right set of paths in use_clang. This should fix the issue. If it doesn't I'll need someone who can reproduce to dig more into why it doesn't.
Ah, I see I missed that paths is reused in use_clang, before the call to use_llvm_tool. I'll fix that now.
Rebase. I'll try landing this once the pre-merge checks complete, if there are no objections.
LGTM, with one spelling error. Thanks!
LGTM overall. Just a few nits.
Thu, May 27
Remove unrelated changes.
Upload correct patch.
Remove unrelated parts of diff.
Nearly there. Not approving since there's a build error though...
Wed, May 26
Fix another comment.
Fix some stale comments.
LGTM from a yaml2obj standpoint, but it would be a good idea to rope in someone with XCOFF experience to confirm they are happy with the file format writing before committing this.
LGTM, with nit.
Tue, May 25
Taking the conversation out of line.