Audit Log File

[12345swordy]

The audit log file itself. Current in draft for proof of concept for other people before any further development.
CC @RazvanN7 @dkorpel any suggestions?
Related PR: #12636

[dlang-bot]

Thanks for your pull request and interest in making D better, @12345swordy! 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 coverage diff by visiting the details link of the codecov check)
• 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 run digger -- build "master + dmd#12710"

[12345swordy]

@thewilsonator why the bot label this as need work when this is in draft mode?

[ibuclaw]

@thewilsonator why the bot label this as need work when this is in draft mode?

Probably because all checks fail.

[WalterBright]

I'd still like to see a compelling argument for what use this is. I can't think of a single case in all my years of debugging where a 10,000 line log file of scattershot, unfocused messages would have been helpful.

[12345swordy]

I'd still like to see a compelling argument for what use this is. I can't think of a single case in all my years of debugging where a 10,000 line log file of scattershot, unfocused messages would have been helpful.

Then simply don't use it. The logging system is driven by compiler commands. If you want to see a logging output for a particular function or module then pass said value to the compiler command. The logging system is design to be pay as you go.

As I and others said before, this is part of dlang/project-ideas#79 project.

[12345swordy]

@thewilsonator wrapping the "else if" statement in a debug condition causes the program to not compile. Is this a bug?

[thewilsonator]

Don't think so, you can't interleave compile time and runtime conditionals

[RazvanN7]

@12345swordy @WalterBright seems to be opposing this addition. You need to come with a compelling case for it, otherwise this will not get accepted.

[12345swordy]

@RazvanN7 What I want to know if @WalterBright is against dlang/project-ideas#79 as an concept in general, before I pursue this pr any further. As of right now, I have lost all my motivation regarding this PR.

[maxhaton]

I'd still like to see a compelling argument for what use this is. I can't think of a single case in all my years of debugging where a 10,000 line log file of scattershot, unfocused messages would have been helpful.

In debug builds at least, having all logging merely a branch away versus turned off via conditional compilation would actually be quite useful if done properly. Currently tracking down what the compiler is doing when debugging (i.e. the stage in between guessing and actually trying a specific fix) is basically a mixture of undoing a bunch of random printfs which may or may not have enough detail or some static ifs.

For release builds that we ship I don't think we need logging.

That being said however it is very common now to look into the compilers state when discussing code (e.g. looking at LLVM IR, or GCC GIMPLE/RTL etc.), so a more something along these lines would be good (e.g. dumping the AST in between semantic operations, tracking what changes).

[ibuclaw]

That being said however it is very common now to look into the compilers state when discussing code (e.g. looking at LLVM IR, or GCC GIMPLE/RTL etc.), so a more something along these lines would be good (e.g. dumping the AST in between semantic operations, tracking what changes).

Right, logging to console by default is the wrong approach. You want each logical component (The current Visitors are a good marker for a boundary) to write to its own stream.

[12345swordy]

All green now.

[12345swordy]

@RazvanN7

[dkorpel]

This PR is hard to review because it introduces a piece of reusable code with 0 users. @WalterBright doesn't see the use of this. I see several problems with the current practice of printf calls in code comments, but this PR doesn't address those yet. I appreciate your effort and am sorry to say this after this has gone back and forth in code review for so long, but I think this is the wrong approach. This PR could get in under the "72h no objection -> merge" label, but as soon as those printfs actually get replaced by the new logging function, it will be under Walter's scrutiny.

So how could we move forward? My proposal is:

1. Get Walter's approval of changing //printf into anything else.
   If that gets vetoed right away, it's dead in the water unfortunately.

2. Incrementally replace //printf(...) into debug dbg(...); (or whatever forms gets agreed upon)
   The dbg function is simply a wrapper around printf:

void dbg(T...)(string fmt, T args, string file = __FILE__, int line = __LINE__) 
{
    printf(fmt.toCstring, args);
}

3. Add features to dbg, e.g.:

• automatically calling toChars() on arguments
• prefix with __FILE__ and __LINE__ of the source of the call
• filtering logs by module / function name, either of the compiler or the compiled code
• outputting to a log file

[12345swordy]

The reason why @ibuclaw told me to output to a file instead of outputting to the console, is because he doesn't want me to replace the ```//printf```` statements that walter uses. Thus there are no plans of replacing the printf statements scatter around the dmd source files.
Don't get me wrong, I want to replace the printf statements initially, but @ibuclaw had talk me out of it over beerconf.

[dkorpel]

Thus there are no plans of replacing the printf statements scatter around the dmd source files.

How is audit_log.d going to be used then?

Don't get me wrong, I want to replace the printf statements initially, but @ibuclaw had talk me out of it over beerconf.

What was his reason?

[12345swordy]

That doesn't answer my question. Edit: to clarify, I'm not asking how it won't be used, I'm asking how it will be used.

See here under the "How does this project help the D community?" section.

[dkorpel]

How is audit_log.d going to be used then?

See here It is going to be use for debugging dmd, and figuring out how dmd works for newcomers.

That project still mentions replacing printf statements with the log statements. If you're not doing that, then are you going to write log statements in addition to the printf statements?

[12345swordy]

That project still mentions replacing printf statements with the log statements.

That was before I had talk with @ibuclaw regarding requirements.

If you're not doing that, then are you going to write log statements in addition to the printf statements?

No, I am just leaving the printf stuff alone to Walter, while writing log statements, per requirement set by other people.

If you have any further questions regarding the reasoning behind the requirements then contact @ibuclaw and @maxhaton via email please.

[dkorpel]

No, I am just leaving the printf stuff alone to Walter, while writing log statements, per requirement set by other people.

Okay, so the project page is outdated, and based on verbal discussion these requirements were established:

• must be able to print to a file instead of console
• leave //printf alone

That still doesn't explain where in dmd's source audit_log is going to be used and to what end. Let's say for example: it's going to be used to log expression semantic. Currently there's these statements everywhere:

static if (LOGSEMANTIC)
{
    printf("ImportExp::semantic('%s')\n", e.toChars());
}

Are you going to write log statements in addition to that? Are you only going to log things that don't have printf logging yet? Will log calls be inserted externally / through a debugger?

[maxhaton]

I think the best place to put these is in between "passes" (or the closest thing dmd has to passes) a la GCC.

The printfs should be replaced by something else. Walter has previously said that a large dump of info is not useful, I think this is false - although the info should be structured better than a mere printf.

[12345swordy]

No, I am just leaving the printf stuff alone to Walter, while writing log statements, per requirement set by other people.

Okay, so the project page is outdated, and based on verbal discussion these requirements were established:

* must be able to print to a file instead of console

* leave `//printf` alone

That still doesn't explain where in dmd's source audit_log is going to be used and to what end. Let's say for example: it's going to be used to log expression semantic. Currently there's these statements everywhere:

static if (LOGSEMANTIC)
{
    printf("ImportExp::semantic('%s')\n", e.toChars());
}

Are you going to write log statements in addition to that? Are you only going to log things that don't have printf logging yet? Will log calls be inserted externally / through a debugger?

Look at this PR discussion please. I have tried replacing them, only for it to be rejected.
#12636

[dkorpel]

Look at this PR discussion please. I have tried replacing them, only for it to be rejected.

That doesn't answer my question.
Edit: to clarify, I'm not asking how it won't be used, I'm asking how it will be used.

[12345swordy]

Look at this PR discussion please. I have tried replacing them, only for it to be rejected.

That doesn't answer my question. Edit: to clarify, I'm not asking how it won't be used, I'm asking how it will be used.

See here under the "How does this project help the D community?" section.

[dkorpel]

See here under the "How does this project help the D community?" section.

I'm not asking about the high level goal of a logging system, I'm talking about this PR and audit_log.d specifically. Give me a spot in DMD (file and line number) where I could expect a call to dmd.audit_log.Log() in the future.

[12345swordy]

See here under the "How does this project help the D community?" section.

I'm not asking about the high level goal of a logging system, I'm talking about this PR and audit_log.d specifically. Give me a spot in DMD (file and line number) where I could expect a call to dmd.audit_log.Log() in the future.

#12710 (comment)

[dkorpel]

That doesn't give a file and line number.

[12345swordy]

That doesn't give a file and line number.

That is completely irrelevant to this PR here.

@dkorpel If you have further question on where the longterm goal of this PR please email @maxhaton or @ibuclaw. I have more then enough address enough with this PR.

[12345swordy]

"How I would use this" is not relevant to this PR. The usage of this PR will be implemented separately from this PR. Asking for example usage of this makes no sense.

[dkorpel]

That is completely irrelevant to this PR here.

It's absolutely relevant. This PR adds a module with dead code. It introduces functions that are never called, let alone tested. There's no documentation, no bugzilla issue, Walter wants to see a compelling argument for it but the only rationale provided is an outdated issue in dlang/projects with an abstract goal. I can comment on the code style, but this PR as a whole is unreviewable like this.

I tried to gather rationale with the questions in my past comments, but to no avail. The fact that you won't provide me a single potential call site of the Log function does not convince me there is a clear direction for this.

I have more then enough address enough with this PR.

I'm not sure exactly what you mean, but if you're fed up with this, we can close this and let someone else take over with a new PR. If you want to champion this PR, you should provide a single up-to-date piece of rationale for its code changes.

[12345swordy]

That is completely irrelevant to this PR here.

It's absolutely relevant.

No it is not!

I told @maxhaton in the discord server MONTHS AGO that this will be follow up with with separated PRs. Neither @ibuclaw, @maxhaton or even @RazvanN7 object to this. You are the literally only one who object to this PR that is in months of development here.
I am not going to waste further of my time justifying/explaining things to you when I told you REPEATEDLY to email @ibuclaw and @maxhaton for further explanation here regarding design decisions being made behind the scenes here, which you have shown no indication of you doing so.

[12345swordy]

Rant: This here is an example of poor management that discourage me from recommending D to other people here. Dear god no wonder other people have stop contributing to D over the years here, and move to other languages here. I am beating my head against the wall. Keep this poor management up and you kiss good bye to the properties rework that I am in current development.

[pbackus]

I am not going to waste further of my time justifying/explaining things to you when I told you REPEATEDLY to email @ibuclaw and @maxhaton for further explanation here regarding design decisions being made behind the scenes here

IMO this is the problem here—the fact that design decisions are being made "behind the scenes," without any written record, rather than out in the open.

I understand how frustrating it is to repeat yourself over and over about this kind of thing, believe me, but the solution to that problem is to write things down publicly. Not only does this make it easier for a third party like Dennis to review, it makes it easier for anyone doing code archaeology in the future to understand how and why decisions were made.

To give a concrete example: a while ago, I tried to track down the rationale behind why we have both is(foo == module) and __traits(isModule, foo). Ultimately, the trail of clues ended at this PR comment:

It was decided to have both.

Decided by who? For what reasons? The world will never know. The decision was made in private, nobody wrote anything down about it, and the knowledge is now lost forever.

[12345swordy]

@pbackus Fair point on this. I have cool my head down on this already, and I apologize to @dkorpel if I were rude or aggrasive to him.

I originally plan to replace printf statements with this until, I had talk with @ibuclaw via discord and via beerconf and told me to leave printf alone and just output the results to a file and have the file be define by module and by function call when initially design this.
See initial comment that started the conversation with him here:
#12636 (comment)
I plan to use to see what expressions that is passing to a function and what value is it returning from that function I.E.

StringExp semanticString(Scope *sc, Expression exp, const char* s)

Try to see the string value of Expression exp via .toChars() and try to see what it is returning, I.E.

return se;

Most importantly for me is backtracking the flow of log when the compiler had error as it is hard to backtrack when you have a visitor pattern implementation.
The reason I am pursuing this project is due to mediocre VC debugging support compare to C# and C++ for visual studio.
I had to spam printf everywhere just to understand why my code isn't running when developing properties for the d language. This is undesirable as I have to uncomment printf and recomment printf ever time I submit a git commit for my properties PR.

Regarding the rational regarding why leaving printf alone and to output to a file. I honestly can't help you there, as I never ask @ibuclaw why on this and just do as I been told to do.

[dkorpel]

and I apologize to @dkorpel if I were rude or aggrasive to him.

No worries, I understand your frustration.

You are the literally only one who object to this PR that is in months of development here.

The main objection comes from Walter (#12710 (comment)), I'm actually in favor of the project as a whole. I want to prevent the following situation:

• This gets merged
• Follow up PRs start inserting Log statements at various places in DMD
• Walter notices and halts it. "We already have -v, -vcg-ast, and //printf, why yet another system?"
• "just e-mail these people" or "trace back these old conversations on GitHub/Discord" is not going to cut it
• audit_log.d will linger like astbase.d and no one gets what they want

That's why I need a concrete direction and scope of this project, so it can get approved by Walter. Chances are if he doesn't want to replace //printf, he doesn't want to end up with a mix of //printf and Log() either.

StringExp semanticString(Scope *sc, Expression exp, const char* s)

Thanks for the specific example. semanticString has no printf yet, which supports my question "Are you only going to log things that don't have printf logging yet?".

This is undesirable as I have to uncomment printf and recomment printf ever time I submit a git commit for my properties PR.

I agree

Regarding the rational regarding why leaving printf alone and to output to a file. I honestly can't help you there, as I never ask @ibuclaw why on this and just do as I been told to do.

This is important for you to know, because if you blindly follow a rule like that, you may technically comply, but still fail to address the underlying issue. If the underlying issue is "Walter doesn't want log statements other than //printf littered throughout DMD", then sneaking in Log statements where there isn't a printf yet still won't work. That's why I still think we should ask Walter to what extend this project is allowed, and then work within the constraints of that (#12710 (comment)).

[12345swordy]

In discord

Me: Do you know why @ibuclaw wants me to leave the printf statements alone when developing the log pr?

Max: Aim to add infrastructure to mimic what GCC does (look at GDC on godbolt.org to see what I mean), the printfs require either an extension to that or a different approach entirely
And just shove some logging with the various visitor functions just show the concept

"This is important for you to know, because if you blindly follow a rule like that, you may technically comply, but still fail to address the underlying issue. "

Welcome to my work place where you are required to follow rules and not ask any question regarding the rational behind the requirements.