User Details
- User Since
- Jan 18 2017, 2:49 AM (349 w, 5 d)
Tue, Sep 26
Mon, Sep 25
One remaining comment, then looks good to me
Fri, Sep 22
Could you not just modify nonMicrosoftDemangle to handle the dot prefix? That would mean this works for all tools, including llvm-cxxfilt, rather than needing this and the separate D159539 patch. I'm assuming that a dot prefix in front of a Microsoft mangled name doesn't make sense...
This patch should have been created using a GitHub Pull Request. See the big red notice at the top of the page...
Tue, Sep 19
Update second bullet to match proposal.
Updated patch based on my latest proposal. Feedback appreciated!
Sun, Sep 17
Strong +1 to what both @ikudrin and @dblaikie have said: these files on their own don't do anything useful (not even add a test), so they shouldn't be added on their own in a separate commit/review. Please merge them into the review that uses them, or create the test that uses them, but showing the old behaviour instead (I think the former is my preference in this context).
Thu, Sep 14
I realised this dropped off my radar for a while, for various reasons, so I'm picking this back up in an effort to get a consensus and to land something. I'm not bothering uploading a diff at this point, but my most recent proposal was:
Wed, Sep 13
LGTM. Probably should wait for @MaskRay too.
Sorry for the delay - Phabricator issues and being busy meant I only just got back to this.
Mon, Sep 11
Fri, Sep 8
My opinion hasn't changed that it's a mistake to put the dot demangling in llvm-cxxfilt specifically. That will mean you'll need to make the same change in llvm-nm, llvm-readelf, llvm-objdump and probably other tools too. Code duplication is bad. The next tool to be added might not realise that they can't just use the main demangler library as-is to do their name demangling.
Thu, Sep 7
There look like there are a few of my previous comments that haven't been addressed yet?
Wed, Sep 6
LGTM, with 2 comment nits and one small test issue that should be addressed before merging.
Tue, Sep 5
Mon, Sep 4
Only a couple of nits remaining from me.
@sepavloff, what's the situation with this patch? Are you planning on addressing review comments or similar, or waiting for an upstream patch to land etc etc?
Sep 1 2023
LGTM. Please wait for @MaskRay too.
Looks reasonable to me, although it's been a while since I last looked at this sort of area.
Aug 31 2023
Don't forget to add any new options to the llvm-objdump command guide.
LGTM too, pending @MaskRay's requested fix.
Aug 30 2023
I did a quick skim of comments and the like, but I don't really have the time to read up on BTF, so can't contirbute anything more meaningful really, sorry.
LGTM.
Aug 29 2023
LGTM, with one remaining nit.
Just some nits left from me.
Okay, thanks for the comments. I was trying to see if there was a sensible middle ground that would satisfy both my concerns and those of @MaskRay, but I don't think there really is at this point. Please could you revert to the previous llvm-nm implementation and drop the AIXImportFile stuff (make sure to address the test comments too). Sorry for the churn.
Aug 25 2023
Aug 24 2023
Functionally, the change does what it says it does, and I have no issues with that. However, the discussion in the ticket seems to suggest this isn't a general issue in Darwin, but rather something about your system setup. That needs resolving before this lands.
Makes sense to me, but perhaps one additional test case needed. I'll leave @dschuff to give final approval though.
Aug 23 2023
I've added a few more nits. I've also gone through and highlighted a number of cases that I'm pretty confident don't have existing test coverage. I shouldn't have had to go through these bits myself and highlight all of this - you should have done this yourself, prior to the patch even going up for review. Requesting changes to clear the "Accepted" state.
Aug 22 2023
I took a break away from this for a few days, and decided that I shouldn't block this patch based on the behaviour change. However, I do think the specific case that can now hang (i.e. the case that caused the test to fail) should be highlighted in the release note, so that users can have a hint as to why their script started hanging after they updated to the latest llvm-symbolizer. LGTM with that.
Looks good (should it have had a unit test?). Not sure why this was landed without review though?
Sorry, I've been trying to focus on other things for the majority of the last few days, as LLVM reviewing has been taking up too much of my worktime recently.
Aug 18 2023
I'd just like to make sure I follow what is going on here:
Aug 17 2023
I'm not really seeing from your example why you need --match-full-lines at all. Are you just looking for the {{^}} and {{$}} regex patterns?
What's the motivation for this change?
Looks like clang-format is complaining. Best make sure you've reformatted everything you've added using it.
Aug 15 2023
I'm going to accept this change, although I still have significant concerns about how the whole parsing logic seems more complicated than it needs to be.
Aug 14 2023
Right, that's what I figured was the issue, but could you answer this part of my concern:
Aug 13 2023
As far as I can tell, the only "fix" you've made since the version that caused the hangs was to the test itself. That means that llvm-symbolizer could still hang under certain conditions surely, when it didn't used to...
LGTM, thanks. I think it would be a good idea for @MaskRay to take another look given the fairly significant changes that happened since his approval.
Aug 11 2023
I'll be honest, I'm not convinced the test coverage looks to be anywhere near comprehensive enough. However, I also haven't attempted to review it versus the code changes to check how much of the new code is actually covered. I'd suggest you consider using a code coverage tool to see how much coverage there is of your new code.
Aug 10 2023
LGTM.
clang-format is complaining in the pre-merge CI.
Windows pre-merge CI is still failing with a test related to this patch.
Aug 9 2023
Thanks for addressing my concerns. LGTM.
LGTM, with nits.
Aug 8 2023
Generally looks fine, but there are a number of small issues, and I'm not convinced by the JSON output behaviour change, hence requesting changes to clear the "accepted" state.
One nit from me, otherwise LGTM too.
Aug 7 2023
LGTM. Thanks for the improvement!
Aug 1 2023
LGTM. I'd add the unit test, but I'm not going to push strongly for it.
Jul 31 2023
Just as a heads-up, I'm off for the rest of the week. Hopefully I'll be able to get to any final points either before the end of my workday or later next week.
Oh, the pre-merge check is failing on Windows too for a test from this patch.
I'm off for the next few days, so I'm happy with this once the islower issue has been addressed (in a separate patch) and my comments addressed (in this patch).
I think the change makes sense, but I don't really know anything about debuginfod, so can't really be confident.
LGTM.
No real objections from me to the latest version. I'll put a LGTM, but it should get reviewed by someone more familiar with CHPE and related things before you land it.
Jul 30 2023
Jul 28 2023
Only nits left from me, but someone with more familiarity with this stuff should review it.
Thanks, LGTM.
LGTM too.
Jul 27 2023
My apologies for being slow in getting back to you - I had some time off and then have been busy catching up on all sorts of other reviews. By the way, feel free to ping the thread if it goes stale for a week.
LGTM. Perhaps worth a release note, and probably should wait for @jobnoorman to confirm.
LGTM, with nits addressed.
Jul 26 2023
Unfortunately, my knowledge of COFF-related things is probably insufficient to review chunks of this code, so it would be best if you could get somebody else to review too, maybe @mstorsjo?
Nearly there. Just a couple of small points now from me.
Jul 25 2023
Thanks! LGTM.
This is definitely a big step in the right direction for llvm-objdump, in my opinion, and I look forward to further work in this area. My one concern with this patch is the Mach-O-specific parameter being passed into printPrivateHeaders, as that feels a bit ugly to me. I wonder if we could create the Mach-O dumper somehow with that parameter set in some manner? It would then no longer be a parameter of printPrivateHeaders that impacts all tools. Alternatively, there could be some other mechanism to give the dumper subclasses access to various configuration parameters.
Just chiming in here to give my 2p. lit already has the ability to execute python scripts (call %python), but any such scripts are not directly tied into the lit, so running them etc won't produce output that fits with the lit diagnostics, as I understand it. As I understand it, this patch is essentially just an improvement of the "execute arbitrary python script" option. I'm in favour of it, but if it isn't already clear, I think it would be worth outlining in this patch why simply calling an external python script (which could actually be embedded within the file) is insufficient for the motivating case.