Skip to content

3C JSON output does not escape backslashes in Windows file paths in string literals #619

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

Closed
mattmccutchen-cci opened this issue Jun 18, 2021 · 2 comments · Fixed by #626
Closed
Assignees
Labels
windows failure A bug causing a Windows-specific failure in the regression tests

Comments

@mattmccutchen-cci
Copy link
Member

In the json_formating.c test on Windows, constraint_output.json_final_output.json contains text like this:

{"line":"C:\Users\correct\source\repos\checkedc-clang\clang\test\3C\json_formating.c:8:6:6","Variables":[{"PointerVar":{"Vars":["a_0"], "name":"a"}}]}

This is invalid JSON because of the unescaped backslashes in the file path. Kudos to #605 for detecting this bug, even if it makes more work for us to get the test passing on Windows before we can submit a PR to Microsoft.

Now, is it time to use a real JSON library to generate the files to hopefully prevent this kind of problem forever? The LLVM monorepo already contains its own JSON library; I ran across this when I was fixing the clangd3c build, because clangd uses JSON for the Language Server Protocol. Or we could try to manually fix only the fields we know are a problem now to unblock the submission to Microsoft and leave the churn of the full migration to the JSON library for later.

For convenience of testing the problem on Linux, I made a json-formatting-windows branch that changes the json_formating.c test to use a header file with a backslash in the name. (A backslash is an ordinary character in file paths on Linux, though parts of the LLVM codebase have trouble with it.) We probably can't merge this test to main in its current form because the backslash may cause problems with checking out the repository on Windows, but if we want to keep test coverage for the problem on Linux, we could have a test marked UNSUPPORTED: windows that uses a RUN command to create a file with a backslash at runtime. I'd be happy to write this test if it would be helpful.

@mattmccutchen-cci mattmccutchen-cci added the windows failure A bug causing a Windows-specific failure in the regression tests label Jun 18, 2021
@mattmccutchen-cci mattmccutchen-cci self-assigned this Jun 19, 2021
@mattmccutchen-cci
Copy link
Member Author

Per discussion with Mike, I'm going to do the partial fix to unblock the Windows test as soon as I can.

@mattmccutchen-cci
Copy link
Member Author

I'm now using this issue to refer to the minimal fix to make the test pass on Windows. #620 is for the complete migration.

mattmccutchen-cci added a commit that referenced this issue Jun 19, 2021
Windows.

Other escaping bugs may remain; #620 is to fix all of them.

Fixes #619.
mattmccutchen-cci added a commit that referenced this issue Jun 19, 2021
Windows.

Other escaping bugs may remain; #620 is to fix all of them.

Fixes #619.
mattmccutchen-cci added a commit that referenced this issue Jun 19, 2021
Windows.

Other escaping bugs may remain; #620 is to fix all of them.

Fixes #619.
mattmccutchen-cci added a commit that referenced this issue Jun 19, 2021
Windows.

Other escaping bugs may remain; #620 is to fix all of them.

Fixes #619.
mattmccutchen-cci added a commit that referenced this issue Jun 21, 2021
Windows.

Other escaping bugs may remain; #620 is to fix all of them.

Fixes #619.
mattmccutchen-cci added a commit that referenced this issue Jun 22, 2021
Windows.

Other escaping bugs may remain; #620 is to fix all of them.

Fixes #619.
mattmccutchen-cci added a commit that referenced this issue Jun 23, 2021
…indows. (#626)

* Fix escaping bugs that currently affect the JSON formatting test on
Windows.

Other escaping bugs may remain; #620 is to fix all of them.

Fixes #619.

* Add test of a backslash in a file path on Linux and Mac OS X.

While I'm here, fix a typo in the name of json_formating.c and add `--`
to its `3c` command lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
windows failure A bug causing a Windows-specific failure in the regression tests
Projects
None yet
1 participant