LGTM, thanks, but please give @ikudrin/@MaskRay/... a few days to make any other comments.

One remaining comment, then looks good to me

In D139864#4650016, @DiggerLin wrote:

In D139864#4649934, @jhenderson wrote:

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.

as I mention before, If I modify in `nonMicrosoftDemangle' as

bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
  char *Demangled = nullptr;

  // Not consider the prefix dot as part of the demangled symbol name.
  if (MangledName[0] == '.') {
    ++MangledName;
    Result = ".";
  }

  if (isItaniumEncoding(MangledName))
    Demangled = itaniumDemangle(MangledName, nullptr, nullptr, nullptr);
  else if (isRustEncoding(MangledName))
    Demangled = rustDemangle(MangledName);
  else if (isDLangEncoding(MangledName))
    Demangled = dlangDemangle(MangledName);

  if (!Demangled)
    return false;

  Result += Demangled;
  std::free(Demangled);
  return true;

The llvm-nm or llvm-cxxfilt for machO will demangle "_._Z3f.0v" as ".f.0()"
but
[zhijian@krypton src]$ /opt/at15.0/bin/c++filt _._Z3f.0v
_._Z3f.0v

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...

Update second bullet to match proposal.

In D151187#4647962, @barannikov88 wrote:

When casting, use static_cast, reinterpret_cast, and const_cast rather than C-style casts. There are two exceptions to this:

• When casting to void to suppress warnings about unused variables (as an alternative to `[[maybe_unused]]`). Prefer C-style casts in this instance.
• When casting between integral types (including enums that are not strongly-typed), functional-style casts are permitted as an alternative to static_cast.

Reads good to me as well.

In D151187#4647916, @jhenderson wrote:

Updated patch based on my latest proposal. Feedback appreciated!

The changes look different from the proposal. Am I missing something?

Updated patch based on my latest proposal. Feedback appreciated!

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).

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:

LGTM. Probably should wait for @MaskRay too.

Sorry for the delay - Phabricator issues and being busy meant I only just got back to this.

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.

There look like there are a few of my previous comments that haven't been addressed yet?

LGTM, with 2 comment nits and one small test issue that should be addressed before merging.

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?

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.

Don't forget to add any new options to the llvm-objdump command guide.

In D149757#4630137, @MaskRay wrote:

I wonder whether these files should go to cross-project-tests/

LGTM too, pending @MaskRay's requested fix.

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.

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.

In D158004#4616402, @MaskRay wrote:

In D158004#4616388, @jhenderson wrote:

In D158004#4616384, @MaskRay wrote:

Changing llvm::identify_magic to recognize all files starting with #! as aix_linker_import_file is not correct. Many can well be used as shebang for shell or other scripts.

case '#':
  if (Magic[1] == '!')
    return file_magic::aix_linker_import_file;
  break;

How would you suggest that @DiggerLin proceeds then? The impact of misidentifying a file with a shebang as an AIX import file is that tools will no longer report "invalid object file" or something to that effect for such files, but will otherwise do nothing with them, so it seems relatively harmless to me.

Sorry, I have analyzed the AIX use cases... I just noticed this patch and am concerned that identify_magic mischaracterizing files with shebang as AIX-specific would likely cause beyond-negligible negative impacts...
identify_magic in LLVMBinaryFormat is somewhat commonly used outside of llvm-project.

In D158004#4616384, @MaskRay wrote:

Changing llvm::identify_magic to recognize all files starting with #! as aix_linker_import_file is not correct. Many can well be used as shebang for shell or other scripts.

case '#':
  if (Magic[1] == '!')
    return file_magic::aix_linker_import_file;
  break;

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.

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.

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.

In D158164#4595420, @strimo378 wrote:

--match-full-lines is require to check that nothing trailing is missing. Current approach is to add in 100 lines {{$}}

I'd just like to make sure I follow what is going on here:

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.

In D144872#4584865, @DiggerLin wrote:

In D144872#4579160, @jhenderson wrote:

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.

In the test big-archive-xcoff-auxi-head-align.test , it test whether the XCOFF object data member which has auxiliary header align correctly based on the information of auxiliary header. it is enough here. In AIX OS since build bot use CMake 3.22, it use llvm-ar instead of AIX OS ar. it will test the functionality too. (we have to add the -DCMAKE_AR=/usr/bin/ar when we compile, since llvm-ar do not align XCOFF object correctly for big archive in AIX os ). after the the patch commit, we will use the llvm-ar instead of the AIX ar

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.

In D157210#4584620, @sepavloff wrote:

In D157210#4584361, @jhenderson wrote:

My opinion is that if someone is in interactive mode and they get an error, it may make sense to exit llvm-symbolizer, rather than continue. How does this differ to the previous (prior to your patch) behaviour for other error cases (i.e. cases other than "missing an input file/address pair")?

This is behavior of llvm-symbolizer before this patch. Whenever it detects an invalid input it echoes the input and continue waiting on stdin. This behavior makes sense, - if the program got wrong command from interactive input, it can continue working because the next commands may do useful job.

Anyway, it is not llvm-symbolizer that caused the hang, it is blocking read in the test script, which waits input from the stream that is empty in the case of new llvm-symbolizer.

Right, that's what I figured was the issue, but could you answer this part of my concern:

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.

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.

LGTM.

clang-format is complaining in the pre-merge CI.

Windows pre-merge CI is still failing with a test related to this patch.

Thanks for addressing my concerns. LGTM.

LGTM, with nits.

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.

In D154987#4559192, @jdenny wrote:

Thanks for the additional comments from everyone.

In D154987#4508046, @ldionne wrote:

It looks like there's no reliable way to test what I was trying to test, and that test isn't *that* important anyway. I'll remove it since it was broken in the first place, so just rebase your patch. Sorry for the trouble.

No worries, and thanks for the fix. I finally got around to the rebase.

In D154987#4539229, @awarzynski wrote:

In D154987#4531662, @jhenderson wrote:

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.

+1

I just extended TestingGuide.rst. Let me know if it's not what you are looking for.

LGTM. Thanks for the improvement!

LGTM. I'd add the unit test, but I'm not going to push strongly for it.

In D142660#4541406, @jhenderson wrote:

In D142660#4538936, @DiggerLin wrote:

As an alternative (but I think adds unnecessary complexity, due to needing an extra variable), you could have both tools read the environment variable into a string variable, then, if the -X option is present, overwrite that variable, and finally feed that string into the parsing code that converts into a BitMode value. If the string is invalid, the parsing code could report an error along the lines of "invalid OBJECT_MODE or -X option value".

if I do not think it is better than my current implement, If I implement as your suggestion, I need another variable to recoded the where the string come from(from OBJECT_MODE or -X option value). otherwise when the invalid value of string , how can I report it is an invalid OBJECT_MODE" or "invalid -X option value"

I'm not convinced you need to distinguish the two. If a user hasn't specified the -X option, then they'll know it's the OBJECT_MODE environment variable. Even if they have both, it should be obvious to a quick glance of the full error message what the wrong value is (because you'd include it in the message) and you could then it won't take long to find which of the two is wrong (even if they don't know that -X trumps OBJET_MODE). That being said, I still prefer the parse OBJECT_MODE first, report an error, then later read the -X option and check for an error there.

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.

Only nits left from me, but someone with more familiarity with this stuff should review it.

Thanks, LGTM.

In D142660#4538936, @DiggerLin wrote:

As an alternative (but I think adds unnecessary complexity, due to needing an extra variable), you could have both tools read the environment variable into a string variable, then, if the -X option is present, overwrite that variable, and finally feed that string into the parsing code that converts into a BitMode value. If the string is invalid, the parsing code could report an error along the lines of "invalid OBJECT_MODE or -X option value".

if I do not think it is better than my current implement, If I implement as your suggestion, I need another variable to recoded the where the string come from(from OBJECT_MODE or -X option value). otherwise when the invalid value of string , how can I report it is an invalid OBJECT_MODE" or "invalid -X option value"

LGTM too.

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.

In D142660#4535693, @stephenpeckham wrote:

I don't see any reason to check the OBJECT_MODE environment variable if the -X flag is used. What would the error be: "You specified a valid -X flag, but by the way, OBJECT_MODE is set to an invalid value"?

LGTM. Perhaps worth a release note, and probably should wait for @jobnoorman to confirm.

LGTM, with nits addressed.

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.

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.