[PATCH] D110839: [LNT] Added reading nm and objdump paths from env variables

Wed Oct 20 08:42:47 PDT 2021

slydiman added a comment.

In D110839#3074994 <https://reviews.llvm.org/D110839#3074994>, @thopre wrote:

> Please add some tests for the environment variables and possibly for the bug fixes depending on what they are. This should also be split into 4 patches, one for each items mentioned. They are all independent, even if you likely made them in one go to get Linux perf work for you. One important reason for splitting is if an issue is found with one of the items the whole patch might need to be reverted. It also allow for easier cherry-pick for people who maintain their own branch of LNT.
>
> Besides the missing tests, the patch LGTM and I'll be happy to approve it.

Most changes in cPerf.cpp are related to the standalone mode which is not used in the production. This mode is designed only for debug purpose. It cannot break anything.

cPerf.cpp was already designed to get nm and objdump path as the parameter (with default value).

The environment variables are used to

- redefine or specify the full path to the tools nm and objdump;
- set the root path to binaries.

What are the options? I see only 2 options - the variable is missing (current default behavior) or the variable contains a path to the tool which is used as is. What may be wrong in the code `nm = os.getenv('CMAKE_NM', 'nm')`? 
I don't see any purpose of such tests.

It is necessary to add some executable binaries (tools) and probably some profile files to the LNT repository for requested tests. I think it is redundant and very bad idea. It is impossible to design such tests to be valid on any target or at least on most targets.

I suggest to accept this patch and commit it as is.

BTW, I see that LNT tests did not execute automatically. Something is totally broken here around LNT tests and all LNT tests are useless for now. Who should we ping to fix this?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110839/new/

https://reviews.llvm.org/D110839