Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow verbose output to be directed to a file #7746

Closed
wants to merge 1 commit into from

Conversation

marler8997
Copy link
Contributor

@marler8997 marler8997 commented Jan 18, 2018

Provide an option to direct verbose output to a file. This can be used to allow tools to invoke the compiler and capture the verbose output without having to redirect stdout/stderr. This is a potential solution to the rdmd "single invocation problem" and can also be used by other tools to gain information from the compiler without having to redirect stdout/stderr so that error messages and other information still gets back to the user.

EDIT: had originally pushed this to allow rdmd to support single invocation, but now I have another tool that can use it.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 18, 2018

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#7746"

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how this solves anything (whether rdmd reads from stdout or reads from file its still one compiler invocation).

src/dmd/mars.d Outdated
if (!params.verbose)
{
import core.stdc.errno : errno;
error("failed to open verbose output file '%s' (errno %d)", filename, errno);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know that there's a libc function that returns an error message for the given errno.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 18, 2018

I'm not sure how introducing a regression warrants a new feature.

@marler8997 marler8997 closed this Jan 18, 2018
@ibuclaw
Copy link
Member

ibuclaw commented Jan 18, 2018

@marler8997 - Please be positive about this.

Please be aware that things are still up in the air regarding your linked discussion thread. Please understand that any sweeping changes whilst discussions are ongoing are to be considered in haste.

@marler8997
Copy link
Contributor Author

I'm not sure how this solves anything (whether rdmd reads from stdout or reads from file its still one compiler invocation).

I've edited the description to explain why it's useful to allow the verbose output to go to a file.

@WalterBright
Copy link
Member

What's wrong with:

dmd -verbose 2> file

? It's what I do.

@marler8997
Copy link
Contributor Author

marler8997 commented Jan 19, 2018

What's wrong with:

dmd -verbose 2> file

I'm a bit confused; -verbose isn't a dmd flag, I assume you meant -v correct? Also, verbose output goes to stdout but you've redirected stderr. Did you mean this?

dmd -v > file

the point of this PR is to allow a tool to invoke dmd while keeping error messages and other compiler information going to stdout and stderr while at the same time also capturing verbose output. In your example, since OPTLINK prints errors to stdout, all error messages from OPTLINK will go into file instead of to the console for the user to see.

Another solution to this problem would be to redirect stdout as you have in your example and then for the tool to read file and try to separate what output came from dmd's verbose information and whatever else happened to go to stdout (such as OPTLINK output), however this solution is more brittle and requires the tool to have alot more logic about the compiler in order to filter the messages properly. Also when I tried this, I lost the syntax coloring from OPTLINK's error messages.

@timotheecour
Copy link
Contributor

timotheecour commented Jan 20, 2018

@WalterBright @marler8997 I'd like this PR regardless of above mentioned OPTLINK bug, but is this related? DigitalMars/optlink#20 Replace error message box by output to stderr #20

@andralex
Copy link
Member

OPTLINK prints errors to stdout

This is definitely a matter with optlink. What other output goes to stdout?

@marler8997
Copy link
Contributor Author

Other than some special categories of verbose output (i.e. -vls, -vgc, etc) that's all that I know of. If a user wanted to use rdmd and see output from those arguments then we'd have to address those as well.
It's also possible that other linker's output their error message to stdout as well but I can't say that I know.

@andralex
Copy link
Member

optlink would need to be fixed to output to stderr, in all likelihood that shouldn't be too hard. @marler8997 can you please file an issue?

There would also be output of pragma(msg, ...) in the compiled program that also go to stdout. I wonder whether that creates a potential security risk as pragmas could output messages that confuse rdmd. cc @CyberShadow @MartinNowak @John-Colvin If that's the case, that's solid motivation for this PR.

@marler8997
Copy link
Contributor Author

why would you think it's a security risk?

@marler8997
Copy link
Contributor Author

rdmd ignores any lines that don't match the regex it is expecting so it's unlikely to confuse rdmd

@andralex
Copy link
Member

I'm not an expert but I figure pragmas could instruct dmd to load dependencies from paths and modules that aren't supposed to be looked at.

@marler8997
Copy link
Contributor Author

marler8997 commented Jan 20, 2018

Interesting. Yes you could most certainly use pragmas to cause rdmd/dmd to load other files (at least with the current design, not with the new -i interface). I didn't think of that....

@WalterBright
Copy link
Member

You can also use https://dlang.org/dmd-windows.html#switch-Xf

The JSON output is supposed to be so other tools can get at the information.

@marler8997
Copy link
Contributor Author

marler8997 commented Jan 20, 2018

I'm not familiar with what the JSON file contains, are you saying the you think rdmd could use the JSON file to get all the information it needs? It needs to know all the modules that were imported, the compiler config file, and all the library files that are included in the compilation.

@WalterBright
Copy link
Member

I'm not that familiar with what the JSON file contains, are you saying the you think rdmd could use the JSON file to get all the information it needs?

I suggest generating the file, and see if it fills the need or falls short. If the latter, it can be improved.

@marler8997
Copy link
Contributor Author

Will do

@wilzbach
Copy link
Member

There is also a dedicated JSON output in the works, because dub has similar problems and already generates such a probing file:

#7521

@timotheecour
Copy link
Contributor

timotheecour commented Jan 20, 2018

  • @WalterBright sorry but the json output is not relevant at all for rdmd. It doesn't contain imports. It also contains a lot of irrelevant info for rdmd (eg serialized function decls), so even if we added the imports (a much bigger PR than this PR), it could make things slower than to just used -v=file. Also it wouldn't solve backward compatibility problem if it expected json output to have this added field (even worse case than the compatiblity breakage of rdmd relying on -i since it'd introduce a change of requirement on an existing flag)

  • @andralex pragma(msg,..) goes to stderr BUT other things will write to stdout:

__ctfeWrite (whenever gets implemented) could write to stdout

-vtls goes to stdout (and has no prefix, eg main.d(5,13): a0 is thread local so if file name containing a TLS variable starts with import (a weird file with a space) then we already have a bug.

  • other fields may print to stdout (-vgc?) (and maybe also printf while hacking/debugging the compiler), and also if/when we use the compiler as a library.

  • OPTLINK linker errors (and fixing optlink won't be backward compatible)

All in all, -v=file would avoid all these corner cases AND make tooling based on parsing -v robust without having to know such intricacies.

  • This PR is so simple and makes sense, it's 100% in line with other things like:
    -dep/-deps=file
    -X/-Xf=file
    -H/-Hf=file
    -D/-Df=file

Let's get it in please, I haven't heard any valid arguments against it.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 20, 2018

All in all, -v=file would avoid all these corner cases AND make tooling based on parsing -v robust without having to know such intricacies.

Different compiler implementations send output to different locations also, which was the entire point of having global.stdmsg. i.e: gdc sends all verbose messages to stderr because stdout is used for -pipe.

@marler8997
Copy link
Contributor Author

Per Walter's suggestion, I've started adding code to extend the existing JSON format to contain the relevant information needed by rdmd. It's not done yet but hopefully will be soon. (#7757).

@WalterBright
Copy link
Member

It doesn't contain imports. It also contains a lot of irrelevant info for rdmd (eg serialized function decls), so even if we added the imports (a much bigger PR than this PR), it could make things slower than to just used -v=file.

The converse argument is providing information to tools via json:

  1. Json pretty much removes the requirement to define yet another file format, of which dmd already has several. Defining file formats is seductively appealing, but they always wind up as a time consuming quagmire. Not the least of which is they tend to get hastily and sloppily designed, much to the frustration of the poor slob coming along later who has to add capability to it while being backwards compatible. The poster boy for that is the awful dmd.conf file, for which I bear full responsibility for its slapdash and incompetent design, and being a compiler guy I have no excuse.
  2. Json makes it trivial for a tool to read and decode the file, in a correct and robust manner. Phobos has a json parser. Let's leverage it instead of every tool developing their own (inevitably slapdash & incompetent) parser.

I agree that speed may be an issue, as the json file can be large with all the other stuff in it. But I would support a switch to pare that down for specific purposes as needed, but it still should be a json file.

@timotheecour
Copy link
Contributor

timotheecour commented Jan 21, 2018

adding the missing info to json and adding a way to control what json outputs is indeed the best solution. We probably need just 1 way to output json though (via existing -Xf, see #7757) instead of 2 ways (-probe, see #7521)

The poster boy for that is the awful dmd.conf file

for things that get automatically generated (like dmd's json output), vanilla json is fine. However for configuration files (eg dmd.conf, dub.json) that are meant to be human edited, ability to add comments is key IMO. A common json extension that allows line comments would be more appropriate for these cases (eg, sublimetext json configuration allows these). It doesn't add much complexity for consumers (eg preprocess json file to remove line comments)

@marler8997
Copy link
Contributor Author

I did want to mention that there is one drawback to the JSON format. It's not a weakness of JSON itself, more that the current JSON format has an organized structure. This organized structure has a performance cost to the compiler. With the verbose output, you can print any information you like at any time and then throw away that information away if you no longer need it. With the JSON format, you need to save the data until the JSON is generated whether or not you need it for anything else.

In the case of the new data I've added for RDMD (https://github.com/dlang/dmd/pull/7757/files), this means that we now have to save a list of all the files that were imported using the "import expression". Before this, that information was "thrown away" after the file was imported, however, now it is saved to an array of "contentImports" in a field added to the Module class, so that the JSON generation code will have that information. In this particular case it's not a big deal since import expressions are so rare, but it is a limitation to consider when deciding whether we want rdmd to use the JSON format instead of the current "verbose" output.

@marler8997
Copy link
Contributor Author

Going to close. In my opinion, allowing verbose output to be directed to a file might be a nice feature even if it's not necessary for rdmd, but I don't really have any use cases to determine this. Also since this PR has no activity it's safe to assume no one else see's good justification. If someone does come up with a justification we can revisit.

@marler8997 marler8997 closed this Feb 6, 2018
@WalterBright WalterBright reopened this Jan 9, 2019
@marler8997
Copy link
Contributor Author

?

@ibuclaw ibuclaw added the Phantom Zone Has value/information for future work, but closed for now label Jan 9, 2019
@ibuclaw
Copy link
Member

ibuclaw commented Jan 9, 2019

I'll just put this in the phantom zone.

@ibuclaw ibuclaw closed this Jan 9, 2019
@marler8997
Copy link
Contributor Author

Why did @WalterBright reopen this PR and @ibuclaw why did you close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase Phantom Zone Has value/information for future work, but closed for now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants