1499907 - When fixing windows symbols dmd.py can generate invalid json

Status: RESOLVED FIXED

(Core :: DMD, enhancement)

Description

[Eric Rahm [:erahm]]

Our file processing in dmd.py has a few steps:

#1) open the dmd file
#2) fix the the stacks and dump to a tmp file
#3) replace the original file with the tmp file
#4) carry on with the dmd report

When writing to the tmp file we blindly assume the fixed stacks are properly escaped, so for example we can end up with something like:

>  "DbT": "#00: static void std::vector<int,std::allocator<int> >::_Resize<`lambda at z:\build\build\src\vs2017_15.8.4\VC\include\vector:1487:23'>(const unsigned __int64, class std::vector<int,std::allocator<int> >::resize::<unnamed-tag>) [vs2017_15.8.4/VC/include/vector:1441]"

where we have unescaped '\' characters. When we go to parse the JSON it then fails to parse due to an invalid escape sequence.

Comment 1

[Bobby Holley (:bholley)]

The quote in |extern "C"| was also a problem.

Comment 2

[Nicholas Nethercote [inactive]]

So, `fixSymbols` isn't JSON aware. Conceptually, it just takes a line like 
this:

 "B": "#00: REPLACE_ME"

and replaces that "REPLACE_ME" bit like this:

 "B": "#00: extern "C" foobar"

we need this instead:

 "B": "#00: extern \"C\" foobar"

The cleanest way to do this would be to add an optional parameter to
`fixSymbols` that requests the transformed part be JSON-escaped.

(The alternative would be to do some parsing in dmd.py to find the substring that needs JSON escaping, but that's pretty gross.)

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Add a `jsonEscape` argument to `fixSymbols`

erahm, does this fix things for you?

bholley, you might also like to try it.

Comment 4

[Bobby Holley (:bholley)]

Trying this patch would probably take me about fifteen minutes of fiddling recreate the setup on windows where this happened to me. I'm willing to do that if necessary, but want confirmation that it's really needed. Please NI if that's the case.

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 9026571 [details] [diff] [review]
Add a `jsonEscape` argument to `fixSymbols`

Review of attachment 9026571 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, adding the option to fixSymbols is definitely the cleanest approach. I don't have a windows setup to test against right now, but logically this looks like it'll do the right thing.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to Bobby Holley (:bholley) from comment #4)
> Trying this patch would probably take me about fifteen minutes of fiddling
> recreate the setup on windows where this happened to me. I'm willing to do
> that if necessary, but want confirmation that it's really needed. Please NI
> if that's the case.

I'm going to land the patch. If you have problems with it later on, let me know.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee6e5895b63
Add a `jsonEscape` argument to `fixSymbols`. r=erahm

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/fee6e5895b63