- User Since
- Jan 18 2017, 2:49 AM (192 w, 1 d)
Mostly looks okay. I think there is no testing for the assembly parser being able to parse the new section type?
LGTM, thanks for working on this.
Rename test inputs and add comment.
Generally looks fine, barring nits. I wonder whether it might be easier for downstream providers such as my team to merge this patch in if it were in separate commits for each of the major changes, but I don't really mind.
Looks good, aside from one question.
LGTM, with nit.
A couple of remaining comment suggestions, otherwise I think this looks good.
I'm happy for that little extra complexity. I think it intuitively makes sense, even if it does mean messing about a little in the code.
Tue, Sep 22
Mon, Sep 21
LGTM, assuming I'm not mistaken in thinking there's another patch related to this one.
Do we actually have any non-fatal errors in llvm-objcopy etc currently? If not, I think a better approach would be to simply bubble up the errors via Expected and Error usage, and handle the failures in the client code directly. The client code could then choose to either report them as errors (or warnings) and/or abort what it was doing. I imagine in the case of dwarfutil, you'd be best off aborting optimizing a given CU's debug data if it looked dodgy somehow, in which case, you don't need to distinguish between different classes of errors. This is the approach we take in our downstream port for our debug line rewriting that we've implemented.
My instinct is that I prefer a third approach - simply keep both combined and separate lists - the combined list (could be a vector or an ordered map) is the offset-ordered list of all units, whilst there are then separate lists for compile and type units, with members being pointers to the units stored in the primary list. I think this has the advantage of being very explicit about things, with the drawback being that there are multiple members to keep in sync (possibly alleviated by having a wrapper class around the containers).
Thanks for the reviews. I'll land this later today.
Fri, Sep 18
Maybe worth stating in the description and/or title that you are using CHECK-NEXT explicitly to achieve this. Otherwise, LGTM.
This sounds like a useful thing too. It should be fairly straightforward to test, as it's possible to e.g. have a python script in the test, which gets executed. Could you add test cases, please?
I guess if you wanted to add explicit testing that the behaviour happens regardless of the presence of specific load commands, that would be possible, using a minimal Mach-O file, but I don't mind if you don't want to. LGTM, if you don't.
I might be missing it, but do you have direct testing showing that the default for IncludeDirs\Files\Opcodes is an empty output, when the output is written? I think it's important that this is tested.
I've done a partial review to keep you going. I'm out of time to look at this further today though, especially given how cold I am on the patch. Many of my comments apply to multiple tests, even if I haven't got to those tests.
Thu, Sep 17
Address @grimar's comments (added additional comments, used split-file, renamed various things). Also renamed two tests using split-file which are not specifically .ll or .s.
Wed, Sep 16
I've made two inline suggestions, but I'm not at all familiar with the other code, so can't further assist there.
I've addressed the main issues I aimed to resolve in other patches. There may be some issues with odd output of error messages when the output is interleaved, but I have other priorities to work on at this point. If someone wants to work on this furhter, it's probably best done in a new review.
I'm not convinced this is really correct. After all, a path with mixed separators (e.g. '/my/path\to\foo') could be a Windows path rooted at the current drive. This mixed style can sometimes happen when things get concatenated together, so I don't think it's compeletely unreasonable.
Tue, Sep 15
Please add the new option to the Command Guide documentation.
We should probably have a bit more direct testing for this behaviour, as I agree it's useful and we don't want to accidentally lose the behaviour in a future change. Maybe a unit test, I don't really mind.
On Windows, paths starting with drive letter pattern are absolute.
This isn't a difference is it? That's also the behaviour for is_absolute, I believe?
There seems to be more going on in this patch than the description talks about. In particular, it looks like this patch causes relocations to be printed when they weren't before. I don't think this is necessarily a problem, but it needs explaining in the description at least, and if sensibly practical, should actually be a separate patch, to minimise behaviour changes per patch.
LGTM, with nit.
Mon, Sep 14
LGTM too, thanks!
I think there's probably some value testing every value, as there is a chance that a future refactor could lead to a bug where there is ambiguity between a CORE note value and another variety of note, for example. Having a test for each individual value ensures that any such bug would be picked up. However, I'm not dead set on this idea, and can see counter-arguments to it. LGTM, but happy to defer to others opinion on whether testing each value is atually necessary.
This looks a lot cleaner. LGTM.
Thu, Sep 10
LGTM, with a couple of nits.
Looks good from my point of view, but best wait for the conversation with @dblaikie to be resolved.
Wed, Sep 9
It seems in general that a build system will want a different symbol ordering file per output, so any involving more than one output would need them specified individually. I don't think a global variable that sets it for all links involved in a build (including CMake links) really makes sense as a result. Limiting it down to a single file via any mechanism (whether within the file or command-line option) sounds like a sticking plaster solution rather than actually the right fix. As @MaskRay has already mentioned, the fix is to specify the right options for the right link. I would think the same applies for options like --dynamic-list, since you might want to export different symbols for different links. Usually the way to do this is with a lower-level customisation of linker options.
Tue, Sep 8
No more comments from me, but please give @kzhuravl a chance to respond before pushing this patch.
You should add a [test] tag to the patch/commit title. LGTM, with that and the inline nit.
LGTM with comment update.
@kzhuravl, do you have any additional comments to add at all, since you rejected the original patch?
LGTM too. I noticed that --version (and -V) are missing from the Command Guide for llvm-install-name-tool, along with --help, unlike e.g. llvm-objcopy. Perhaps worth a separate patch if you get a few minutes to fix the doc.