- User Since
- Jul 22 2016, 3:32 PM (160 w, 4 d)
I'll take a closer look at this diff this week, sorry about the delay
Fri, Aug 16
@seiya i think this diff is safe to commit (if it has not been committed yet)
Thu, Aug 15
Wed, Aug 14
in general i like the approach, i have added a few minor comments and one question (maybe I'm missing smth).
Sun, Aug 11
Fri, Aug 9
LG, thanks for working on this!
Wed, Aug 7
Tue, Aug 6
Mon, Aug 5
Thu, Aug 1
Wed, Jul 31
Tue, Jul 30
Mon, Jul 29
Wed, Jul 24
i think the title of the diff should be adjusted a bit: 's/llvm-objcopy/llvm-objdump/g'
Tue, Jul 23
the code looks good to me, but we need tests. I'd add simple tests which take YAML (similarly to how the existing tests work), generate MachO (using yaml2obj), copy it using llvm-objcopy,
dump the results (using llvm-readobj and obj2yamal) and verify that the output is what we expect. Plus those tools will parse the binary generated by llvm-objcopy thus they will verify (to some extent) that we have built a valid MachO object file.
Jul 20 2019
Jul 17 2019
it's simply a very very busy season
sorry about the delay, will get to these reviews today
Jul 15 2019
Jul 14 2019
Jul 3 2019
I've added a couple of minor comments, but other than that - looks good to me) I'd wait for @mtrent 's approval as well
Jul 2 2019
Jun 24 2019
Jun 21 2019
Jun 20 2019
Jun 19 2019
Just to double check - do the existing tests pass with this patch ? If so - LGTM
Jun 18 2019
Regarding tests - even without computing the new layout - could we add a test which simply copies a .so and verifies that the output is the same ?
sorry about the delay with the review, I've been (and probably will be over the next few weeks) quite busy recently, but I'll take a closer look at the diff tomorrow.
Jun 17 2019
i think that we should not "replicate cctools lipo error messages exactly", ours could be better / more convenient or informative where it makes sense.
But in this particular case (see the comments above) I agree, including the names into the error message would be good!
Regarding exit codes - yes, it's important (for many reasons) to make sure that our exit codes are correct.
Jun 16 2019
i think we can commit this
Jun 11 2019
Jun 10 2019
I've added one inline comment, other than that - LGTM, I'd wait for @mtrent as well )
ok, this makes sense to me now, thanks for the explanation. LGTM with a couple of inline comments
Jun 7 2019
Jun 6 2019
khm, could you please explain the rationale for this change ? If yaml2obj completely ignores virtual sections we won't be able to create a MachO binary containing them, moreover, even in some simple cases the round trip conversion yaml -> obj -> yaml will create a completely different binary. Having ability to create binaries with virtual sections overall seems to be useful for testing various tools etc.
the static html file (attached to my previous comments) contains some relative paths to various .css style sheets, so it needs to be placed into <your_build_dir>/docs/html/CommandGuide/ to be rendered correctly
Address @mtrent comments,
I think this looks okay / relatively small change anyway, i don't want to block it anymore, if there are any extra comments I think we can always either revert the diff or address them post-commit.
Jun 5 2019
@seiya, ok, i think if nobody comments on this diff in the coming days it's safe to proceed and commit it, I kinda don't want to block it and I'm looking forward to more tests in the follow-up patches .
Jun 4 2019
so in general this looks very promising to me,
however test coverage needs improvements imo, though given that at the moment we don't expose any non-trivial options here, we can add more tests incrementally. Btw - for DYSYMTAB, SYMTAB - is it possible to add YAML-based tests ? or yaml2obj's support is not sufficient for this ?
Jun 3 2019
May 31 2019
May 30 2019