-
Notifications
You must be signed in to change notification settings - Fork 5
Slight improvements and comments for file path canonicalization (for possible addition to #488) #508
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
Conversation
In particular, getCanonicalFilePath (the version that asserts) is no longer used.
…dc-clang into one_clang_pass.file-path-tweaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments. Getting the right files and directories into 3c when there are also restrictions is difficult and I appreciate your work getting that all set up properly. Do you think we have enough tests at this time?
Request changes solely for an answer to the question about FeAbsS != ToConv
below.
"3C internal error: not writing the new version of this file due " | ||
"to failure to re-canonicalize the file path provided by Clang"); | ||
DE.Report(SM.translateFileLineCol(FE, 1, 1), ErrorId); | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need this code block. As invaluable as this was when we were calling exit()
, diagnostics will be emitted properly when exiting naturally, which is done in 3C.cpp through tool.run() or when diagnostics error count exceeds 0.
And it's within an if
block, which takes care of it.
And it looks like I forgot to remove the block in ProgramInfo.cpp myself after adding the comment below it for why the exit()
is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the block, this DiagnosticBuilder
will still exist during the call to PrintExtraUnwritableChangeInfo
and Clang raises an assertion failure about two DiagnosticBuilder
s existing at the same time. There is another case like this in rewriteSourceRange
. Should we comment every such case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's prior code that didn't have this trouble. Is it about the error together with the note? How does clang (or other) deal with this situation? Does the call to getDiagnostics()
close open builders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's prior code that didn't have this trouble.
Can you point me to this code? That might be the fastest way to clear this up.
Is it about the error together with the note?
No, for the error, the DiagnosticBuilder
returned by DE.Report
isn't saved to a variable, so it gets destructed immediately. The problem is that without the block, the DiagnosticBuilder
for the note is still held in a local variable while additional diagnostics are generated by PrintExtraUnwritableChangeInfo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed the importance of the local variable for both the old and new code. You could consider moving the call to PrintExtraUnwritableChangeInfo
before the builder, but then I'm out of ideas and this is fine as it is. Yes, please add a brief comment on the block if you keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could consider moving the call to
PrintExtraUnwritableChangeInfo
before the builder
I don't want to do that because the note provides values specifically related to this error (I thought it would be too long to put everything in one error diagnostic) while the PrintExtraUnwritableChangeInfo
boilerplate is the same for 4 different error cases.
Yes, please add a brief comment on the block if you keep it.
Done. Maybe in the future, we can use a more concise wrapper function to generate diagnostics and we won't have to worry about these blocks.
PrintExtraUnwritableChangeInfo(); | ||
continue; | ||
} | ||
if (FeAbsS != ToConv) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the purpose of tryGetRealPathName()
above to convert or to check that no conversion is required? The rest of the code only uses FeAbsS
so it seems like this diagnostic is overkill. Maybe leave the note and remove the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We expect tryGetRealPathName
to return a canonical path, meaning that tryGetCanonicalFilePath
would not change it. If the path does change, then the use of the old path elsewhere in the program analysis (e.g., to determine which elements need to be constrained because they are outside the base directory) may have caused incorrect behavior, so it would be good to report an error. It would probably be safe to write the output file anyway if you prefer that; the error (and nonzero exit code) is the important thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'd prefer to have this type of error at some other point in the codebase, though. Like if we can't canonicalize, just exit after the initial clang pass (or before). But I'll remove the hold if you think this is a good place for it to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is a little awkward, but I don't know of a better place where we have access to a list of files that is guaranteed to include any file that 3C might try to modify. It looks like we might have to either get a list of all files in the SourceManager
or somehow hook the process of adding rewrite buffers to the Rewriter
.
If the problem is just that this code is getting so long that it makes it hard to follow the high-level flow of emit
, I can move it to a subroutine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to you now, I ran out of suggestions ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it for now then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments. Getting the right files and directories into 3c when there are also restrictions is difficult and I appreciate your work getting that all set up properly. Do you think we have enough tests at this time?
I think the existing tests are all that we can reasonably write. (Of course, some of them are temporarily XFAIL-ed due to the problem with the diagnostic verifier, but you verified the output manually.) #488 doesn't (intentionally!) make user-visible behavior changes that might merit new tests, so I think the existing tests worked as intended to ensure that #488 maintained the existing behavior.
Since the code I'm proposing to add in this PR guards against cases that currently may not be able to happen (at least I don't know how to trigger them), there isn't a great way to test that code other than my manual test where I temporarily modified the code to change the output of tryGetRealPathName
.
"3C internal error: not writing the new version of this file due " | ||
"to failure to re-canonicalize the file path provided by Clang"); | ||
DE.Report(SM.translateFileLineCol(FE, 1, 1), ErrorId); | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the block, this DiagnosticBuilder
will still exist during the call to PrintExtraUnwritableChangeInfo
and Clang raises an assertion failure about two DiagnosticBuilder
s existing at the same time. There is another case like this in rewriteSourceRange
. Should we comment every such case?
PrintExtraUnwritableChangeInfo(); | ||
continue; | ||
} | ||
if (FeAbsS != ToConv) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We expect tryGetRealPathName
to return a canonical path, meaning that tryGetCanonicalFilePath
would not change it. If the path does change, then the use of the old path elsewhere in the program analysis (e.g., to determine which elements need to be constrained because they are outside the base directory) may have caused incorrect behavior, so it would be good to report an error. It would probably be safe to write the output file anyway if you prefer that; the error (and nonzero exit code) is the important thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have everything addressed now. If you are still happy with the PR, would you like to merge it (since you are in charge of the target branch anyway)? Thanks for working with me on this.
"3C internal error: not writing the new version of this file due " | ||
"to failure to re-canonicalize the file path provided by Clang"); | ||
DE.Report(SM.translateFileLineCol(FE, 1, 1), ErrorId); | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could consider moving the call to
PrintExtraUnwritableChangeInfo
before the builder
I don't want to do that because the note provides values specifically related to this error (I thought it would be too long to put everything in one error diagnostic) while the PrintExtraUnwritableChangeInfo
boilerplate is the same for 4 different error cases.
Yes, please add a brief comment on the block if you keep it.
Done. Maybe in the future, we can use a more concise wrapper function to generate diagnostics and we won't have to worry about these blocks.
PrintExtraUnwritableChangeInfo(); | ||
continue; | ||
} | ||
if (FeAbsS != ToConv) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it for now then.
* prototype without Diagnostics * enableSourceFileDiagnostics * only enable diags once per ast * add fail case * FileEntry path before CanonicalFilePath * Diagnostic errors cause non-zero exit code * setup multipass verifier * remove unused prefixes * delete unused code; create ASTs in seperate function * rework canonical file path * add test refactoring TODOs * :disable diag_verifier tests * Wording Co-authored-by: John Kastner <[email protected]> * Slight improvements and comments for file path canonicalization (for possible addition to #488) (#508) * Slight improvements and comments for file path canonicalization. In particular, getCanonicalFilePath (the version that asserts) is no longer used. * Add comments about blocks around diagnostic generation. Co-authored-by: John Kastner <[email protected]> Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>
Pulled out of #488 (comment) to ease review.
I'd prefer to have at least the two big comments near the
tryGetRealPathName
calls added to #488 to document the changes made there. If you don't want to deal with the diagnostic code changes and the related cleanup at this time, that's completely fine with me; let me know and I'll remove them from this PR.