bug fix: stderr to stdout

[aaliyahnl]

Checklist

• [ ] Language changes are updated in man/adoc/bpftrace.adoc
• [ ] User-visible and non-trivial changes updated in CHANGELOG.md
• [ ] The new behaviour is covered by tests

[ajor]

The reason usage() prints to standard error is that it's often the only message displayed when invalid command line arguments are used, e.g.: https://github.com/bpftrace/bpftrace/blob/1517d1cc9b7436069412d764ae32d34c9e58afd7/src/main.cpp#L633-L636

We should always print some form of message to stderr when there is an error. Whether this means adding new error messages at each point we currently call usage(), or just continuing to have usage() print to stderr, I don't really mind.

--help was later introduced and also calls ends up calling usage() - I agree that in this instance it shouldn't go to stderr.

[aaliyahnl]

I replaced more err statements as requested by jordalgo.

@ajor could you clarify the next step?

Since I replaced the std:cerr statements, I see I can now add error statements each time usage() is called.

1. Is this format acceptable? std:cerr << "message related to error conditions" << endl;

2. Should do this for every call to usage()? You mention the call to usage() in --help as an exception, but I don't see a place that matches this description. I may not be clear on what --help means.

[ajor]

After looking at what other CLI tools do (e.g. nm, ps) and searching online [1] [2], I think we should continue to print usage to stderr when there is an error, but print it to stdout when --help is requested.

This is what I meant by --help calling usage(): https://github.com/bpftrace/bpftrace/blob/1517d1cc9b7436069412d764ae32d34c9e58afd7/src/main.cpp#L613-L615

It's the only caller of usage() which doesn't exit with an error code and should print to stdout. All the other callers of usage() report errors and should print to stderr.

If we're going to keep usage() going to stderr when there are errors, adding extra error messages shouldn't be required for this PR (but would still be nice to have at some point). For reference, here's a good example of how we can output error messages with LOG(ERROR): https://github.com/bpftrace/bpftrace/blob/1517d1cc9b7436069412d764ae32d34c9e58afd7/src/main.cpp#L638-L641

[aaliyahnl]

Thank you for the detailed feedback. I can return my changes in usage() to stderr.

For the --help condition, I am thinking to replace the call to usage() with the contents of the function. Then modify the contents to have stout.

I can add LOG(ERROR) to the other calls to usage() too.

Two follow up questions:

1. Should I return the change in info() too?
2. Would it be easier to make another issue for adding LOG(ERROR) or would it be better to continue this here?

[ajor]

Thank you for the detailed feedback. I can return my changes in usage() to stderr.

For the --help condition, I am thinking to replace the call to usage() with the contents of the function. Then modify the contents to have stout.

Sorry if I'm misunderstanding, but I think we might be able to do it with less code duplication by having usage() accept a std::ostream& argument (usage(std::ostream &out)) like Printer does: https://github.com/bpftrace/bpftrace/blob/1517d1cc9b7436069412d764ae32d34c9e58afd7/src/ast/passes/printer.h#L12

Then we could call it as either usage(std::cout) or usage(std::cerr) depending on where we want the output to go to.

1. Should I return the change in info() too?

Your info change is good - that'll never be printed in response to an error so stdout always makes sense.

2. Would it be easier to make another issue for adding LOG(ERROR) or would it be better to continue this here?

Entirely up to you :)

[aaliyahnl]

I made the requested change, but it seems to be failing many tests. Any recommendation on how to proceed? I can make a new issue for the error statements after this one. Thank you again.

[jordalgo]

@aaliyahnl From the error messages it seems like these are all valid errors that need to be fixed (e.g. calls to usage that need to be updated) and a formatting error.

[aaliyahnl]

Oh, thank you! I've been trouble getting compiling, and wasn't able to see the error message. I'll work on this as soon as I can

[danobi]

Closing due to inactivity

[jordalgo]

I feel like we might have closed this too aggressively. @aaliyahnl Any desire to continue work on this? If not, I'll hand it off.

[aaliyahnl]

Thank you for checking. Please feel free to hand it off! I thought I would be able to continue, but no longer am. Glad someone else is willing.