-
Notifications
You must be signed in to change notification settings - Fork 747
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
[#7176] json.h update #7181
base: main
Are you sure you want to change the base?
[#7176] json.h update #7181
Conversation
- enable quoted and unquoted keys - skip escaped chars (cause of early parse abort)
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!
Looks right to me. See the comments below. Also let's add tests for these things. See existing tests in ./test/lit/metadce
close++; // Skip escaped character | ||
} | ||
close++; | ||
} |
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.
This pattern of skipping escaped characters appears more than once. How about adding a helper function findEndOfQuoted()
?
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.
While doing so, the asserts at the end (next line) could be error checks with Fatal() << "error message"
(so they also work in non-debug builds - asserts are debug-only).
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 added 2 helpers using macros : (used macros to stay consistent with the style of this file)
- pattern of skipping chars
- runtime assert (to take in account release builds), also I am guessing that we need to change all
assert(..)
occurrences inparse
function at least, since all of them are required ?
Now I have a doubt about the "quoted unquoted" keys in json, while testing I used the file referenced here. the said file have keys unquoted, this is allowed by most of the tools out there (browser included) but this doesn't seem to be allowed by JSON RFC.
I added 2 tests to cover the PR.
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'm not sure about the JSON spec, but it makes sense for us (as just another tool) to support patterns that are commonly supported by tools, even if the spec isn't clear there.
src/support/json.h
Outdated
if (!(condition)) { \ | ||
std::cerr << "Assertion failed: " #condition << " at " << __FILE__ << ":" \ | ||
<< __LINE__ << "\n"; \ | ||
std::terminate(); \ |
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 have Fatal for this in utilities.h
, so we can include that and then replace these two lines with Fatal() << "..message..";
Also I think it is best to avoid a C macro for this, and just do if (..) { Fatal() ..
in the code (the downside of a macro is it isn't clear from the name if it is just an assert - and hence disabled in non-assertion builds - etc.)
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.
done.
;; RUN: wasm-metadce %t.wasm --input-source-map %t.map --graph-file %s.json -o %t.out.wasm --output-source-map %t.out.map | ||
;; RUN: wasm-dis %t.out.wasm --source-map %t.out.map -o - | filecheck %s --check-prefix=BIN | ||
|
||
;; Test that sourcemap information is preserved |
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.
This comment looks like it was copied from another test perhaps?
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.
Yes, I removed not need stuff. test files should be minimal by now.
assert(close); | ||
char* close = curr; | ||
skip_escaped_characters(close); | ||
if (!(*close == '"')) { |
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.
if (!(*close == '"')) { | |
if (*close != '"') { |
char* close = curr; | ||
skip_escaped_characters(close); | ||
if (!(*close == '"')) { | ||
wasm::Fatal() << "Assertion failed (close == '\"') "; |
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.
wasm::Fatal() << "Assertion failed (close == '\"') "; | |
wasm::Fatal() << "missing end quote"; |
(because "assertion" suggests it might be a debug-only assertion)
(func $f (export "f") | ||
(nop) | ||
) | ||
) |
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.
Reading the full test now, I see that this does check we don't error during parse, but it doesn't check that we use the string. We do have more specific testing for JSON,
https://github.com/WebAssembly/binaryen/blob/main/test/gtest/json.cpp
Perhaps we can add a test there? Such a test could check that the output of parsing is correct too. Let me know if you want help writing such a test.
This PR introduce 2 updates to json.h (see #7176 ):
Test : after removing asserts and extra modules in names.wast, wasm-metadce seems to produce a valid wasm.
$ wasm-metadce test/spec/testsuite/names.wast -o /tmp/names.wast --graph-file /tmp/names.json -all
unused: export$unused: export$
unused: export$
unused: export$
unused: export$
unused: export$
unused: export$
unused: export$
unused: export$
unused: export$�
nused: export$
unused: export$�
unused: export$
unused: export$
unused: export$(˹˻𔗎⁽₍❨❪⟮﴾︵﹙(⦅❲❴⟦⟨⟪⟬⦇⦉⦕⸢⸤︗︷︹︻︽︿﹁﹃﹇﹛﹝[{「«‘“‹❮
unused: export$)˺˼𔗏⁾₎❩❫⟯﴿︶﹚)⦆❳❵⟧⟩⟫⟭⦈⦊⦖⸣⸥︘︸︺︼︾﹀﹂﹄﹈﹜﹞]}」»’”›❯
unused: export$1/2
unused: export$1⁄2
unused: export$?
unused: export$A
unused: export$U+0041
unused: export$abc
unused: export$~!@#$%^&*()_+`-={}|[]:";'<>?,./
unused: export$
unused: export$
unused: export$ª
unused: export$¶
unused: export$½
unused: export$¿
unused: export$À
unused: export$Á
unused: export$Â
unused: export$Ã
unused: export$Ä
unused: export$Ā
unused: export$Ă
unused: export$Ą
unused: export$Ǎ
unused: export$Ǟ
unused: export$Ǡ
unused: export$Ǻ
unused: export$Ȁ
unused: export$Ȃ
unused: export$Ȧ
unused: export$Ⱥ
unused: export$̈‽̈̉
unused: export$;
unused: export$Ά
unused: export$Α
unused: export$Λ
unused: export$ϓ
unused: export$ϔ
unused: export$А
unused: export$Ӑ
unused: export$Ӓ
unused: export$՞
unused: export$؟
unused: export$܀
unused: export$ߊ
unused: export$ࠡ
unused: export$ࠢ
unused: export$ࠣ
unused: export$ࠤ
unused: export$ࠥ
unused: export$ऄ
unused: export$अ
unused: export$ॲ
unused: export$অ
unused: export$ਅ
unused: export$અ
unused: export$ଅ
unused: export$୳
unused: export$அ
unused: export$అ
unused: export$ಅ
unused: export$അ
unused: export$൴
unused: export$ะ
unused: export$๛
unused: export$ະ
unused: export$༁
unused: export$ཨ
unused: export$ྸ
unused: export$အ
unused: export$ဢ
unused: export$ႜ
unused: export$჻
unused: export$ᅡ
unused: export$አ
unused: export$ዐ
unused: export$፧
unused: export$፨
unused: export$Ꭰ
unused: export$Ꭺ
unused: export$ᐊ
unused: export$ᖳ
unused: export$ᗅ
unused: export$ᚨ
unused: export$ᚪ
unused: export$ᛆ
unused: export$ᜀ
unused: export$ᜠ
unused: export$ᝀ
unused: export$ᝠ
unused: export$ᠠ
unused: export$ᢇ
unused: export$ᤠ
unused: export$᥅
unused: export$ᥣ
unused: export$ᨕ
unused: export$ᩋ
unused: export$ᩡ
unused: export$ᮃ
unused: export$ᯀ
unused: export$ᯁ
unused: export$ᰣ
unused: export$ᴀ
unused: export$ᴬ
unused: export$᷻
unused: export$Ḁ
unused: export$ẛ
unused: export$Ạ
unused: export$Ả
unused: export$Ấ
unused: export$Ầ
unused: export$Ẩ
unused: export$Ẫ
unused: export$Ậ
unused: export$Ắ
unused: export$Ằ
unused: export$Ẳ
unused: export$Ẵ
unused: export$Ặ
unused: export$Ἀ
unused: export$Ἁ
unused: export$Ἂ
unused: export$Ἃ
unused: export$Ἄ
unused: export$Ἅ
unused: export$Ἆ
unused: export$Ἇ
unused: export$ᾈ
unused: export$ᾉ
unused: export$ᾊ
unused: export$ᾋ
unused: export$ᾌ
unused: export$ᾍ
unused: export$ᾎ
unused: export$ᾏ
unused: export$Ᾰ
unused: export$Ᾱ
unused: export$Ὰ
unused: export$Ά
unused: export$ᾼ
unused: export$‑
unused: export$
unused: export$
unused: export$abc
unused: export$abc
unused: export$cba
unused: export$cba
unused: export$⁇
unused: export$⁋
unused: export$₳
unused: export$←
unused: export$∀
unused: export$⌦
unused: export$⌧
unused: export$⌫
unused: export$⍒
unused: export$⍔
unused: export$⍢
unused: export$⍫
unused: export$⍰
unused: export$⍶
unused: export$⍺
unused: export$␈
unused: export$␚
unused: export$␡
unused: export$␦
unused: export$Ⓐ
unused: export$☙
unused: export$❓
unused: export$❔
unused: export$❡
unused: export$⩜
unused: export$⯑
unused: export$Ⱝ
unused: export$Ɑ
unused: export$Ɐ
unused: export$Ɒ
unused: export$Ⲁ
unused: export$⳺
unused: export$⳻
unused: export$⳽
unused: export$ⷶ
unused: export$ⷼ
unused: export$⸎
unused: export$⸏
unused: export$⸐
unused: export$⸑
unused: export$⸮
unused: export$⸿
unused: export$〇
unused: export$〷
unused: export$あ
unused: export$ア
unused: export$ㄚ
unused: export$ㅏ
unused: export$㈎
unused: export$㈏
unused: export$㈐
unused: export$㈑
unused: export$㈒
unused: export$㈓
unused: export$㈔
unused: export$㈕
unused: export$㈖
unused: export$㈗
unused: export$㈘
unused: export$㈙
unused: export$㈚
unused: export$㈛
unused: export$㉄
unused: export$㉮
unused: export$㉯
unused: export$㉰
unused: export$㉱
unused: export$㉲
unused: export$㉳
unused: export$㉴
unused: export$㉵
unused: export$㉶
unused: export$㉷
unused: export$㉸
unused: export$㉹
unused: export$㉺
unused: export$㉻
unused: export$㋐
unused: export$ꀊ
unused: export$ꓮ
unused: export$ꕉ
unused: export$꘏
unused: export$Ꙗ
unused: export$ꙮ
unused: export$ꚠ
unused: export$꛷
unused: export$ꠀ
unused: export$ꠣ
unused: export$꠱
unused: export$ꡝ
unused: export$ꢂ
unused: export$꣪
unused: export$ꤢ
unused: export$ꥆ
unused: export$ꦄ
unused: export$ꨀ
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$︖
unused: export$﹖
unused: export$
unused: export$"
unused: export$?
unused: export$A
unused: export$ア
unused: export$ᅡ
unused: export$
unused: export$�
unused: export$�
unused: export$𐀀
unused: export$𐅁
unused: export$𐅵
unused: export$𐅶
unused: export$𐊀
unused: export$𐊠
unused: export$𐌀
unused: export$𐌰
unused: export$𐎠
unused: export$𐐂
unused: export$𐐈
unused: export$𐒖
unused: export$𐒰
unused: export$𐔀
unused: export$𐝀
unused: export$𐠀
unused: export$𐤀
unused: export$𐤠
unused: export$𐦀
unused: export$𐦠
unused: export$𐦽
unused: export$𐨀
unused: export$𐬀
unused: export$𐰀
unused: export$𐰁
unused: export$𐲀
unused: export$𐹻
unused: export$𑀅
unused: export$𑂃
unused: export$𑄧
unused: export$𑅃
unused: export$𑅐
unused: export$𑆃
unused: export$𑈀
unused: export$𑊀
unused: export$𑊰
unused: export$𑌅
unused: export$𑍰
unused: export$𑐀
unused: export$𑒁
unused: export$𑖀
unused: export$𑘀
unused: export$𑚀
unused: export$𑜒
unused: export$𑜠
unused: export$𑢡
unused: export$𑫕
unused: export$𑰀
unused: export$𑲏
unused: export$𑲯
unused: export$𒀀
unused: export$𖡄
unused: export$𖧕
unused: export$𖩆
unused: export$𖫧
unused: export$𖽔
unused: export$𛱁
unused: export$𛱤
unused: export$𝐀
unused: export$𝐴
unused: export$𝑨
unused: export$𝒜
unused: export$𝓐
unused: export$𝔄
unused: export$𝔸
unused: export$𝕬
unused: export$𝖠
unused: export$𝗔
unused: export$𝘈
unused: export$𝘼
unused: export$𝙰
unused: export$𝚨
unused: export$𝛢
unused: export$𝜜
unused: export$𝝖
unused: export$𝞐
unused: export$𝪋
unused: export$𝪋𝪤
unused: export$𞠣
unused: export$𞥟
unused: export$🄐
unused: export$🄰
unused: export$🅐
unused: export$🅰
unused: export$🇦
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$
unused: export$
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: export$�
unused: func$100
unused: func$101
unused: func$102
unused: func$103
unused: func$104
unused: func$105
unused: func$106
unused: func$107
unused: func$108
unused: func$109
unused: func$110
unused: func$111
unused: func$112
unused: func$113
unused: func$114
unused: func$115
unused: func$116
unused: func$117
unused: func$118
unused: func$119
unused: func$120
unused: func$121
unused: func$122
unused: func$123
unused: func$124
unused: func$125
unused: func$126
unused: func$127
unused: func$128
unused: func$129
unused: func$130
unused: func$131
unused: func$132
unused: func$133
unused: func$134
unused: func$135
unused: func$136
unused: func$137
unused: func$138
unused: func$139
unused: func$140
unused: func$141
unused: func$142
unused: func$143
unused: func$144
unused: func$145
unused: func$146
unused: func$147
unused: func$148
unused: func$149
unused: func$15
unused: func$150
unused: func$151
unused: func$152
unused: func$153
unused: func$154
unused: func$155
unused: func$156
unused: func$157
unused: func$158
unused: func$159
unused: func$160
unused: func$161
unused: func$162
unused: func$163
unused: func$164
unused: func$165
unused: func$166
unused: func$167
unused: func$168
unused: func$169
unused: func$170
unused: func$171
unused: func$172
unused: func$173
unused: func$174
unused: func$175
unused: func$176
unused: func$177
unused: func$178
unused: func$179
unused: func$180
unused: func$181
unused: func$182
unused: func$183
unused: func$184
unused: func$185
unused: func$186
unused: func$187
unused: func$188
unused: func$189
unused: func$190
unused: func$191
unused: func$192
unused: func$193
unused: func$194
unused: func$195
unused: func$196
unused: func$197
unused: func$198
unused: func$199
unused: func$200
unused: func$201
unused: func$202
unused: func$203
unused: func$204
unused: func$205
unused: func$206
unused: func$207
unused: func$208
unused: func$209
unused: func$210
unused: func$211
unused: func$212
unused: func$213
unused: func$214
unused: func$215
unused: func$216
unused: func$217
unused: func$218
unused: func$219
unused: func$22
unused: func$220
unused: func$221
unused: func$222
unused: func$223
unused: func$224
unused: func$225
unused: func$226
unused: func$227
unused: func$228
unused: func$229
unused: func$23
unused: func$230
unused: func$231
unused: func$232
unused: func$233
unused: func$234
unused: func$235
unused: func$236
unused: func$237
unused: func$238
unused: func$239
unused: func$24
unused: func$240
unused: func$241
unused: func$242
unused: func$243
unused: func$244
unused: func$245
unused: func$246
unused: func$247
unused: func$248
unused: func$249
unused: func$25
unused: func$250
unused: func$251
unused: func$252
unused: func$253
unused: func$254
unused: func$255
unused: func$256
unused: func$257
unused: func$258
unused: func$259
unused: func$260
unused: func$261
unused: func$262
unused: func$263
unused: func$264
unused: func$265
unused: func$266
unused: func$267
unused: func$268
unused: func$269
unused: func$270
unused: func$271
unused: func$272
unused: func$273
unused: func$274
unused: func$275
unused: func$276
unused: func$277
unused: func$278
unused: func$279
unused: func$280
unused: func$281
unused: func$282
unused: func$283
unused: func$284
unused: func$285
unused: func$286
unused: func$287
unused: func$288
unused: func$289
unused: func$290
unused: func$291
unused: func$292
unused: func$293
unused: func$294
unused: func$295
unused: func$296
unused: func$297
unused: func$298
unused: func$299
unused: func$300
unused: func$301
unused: func$302
unused: func$303
unused: func$304
unused: func$305
unused: func$306
unused: func$307
unused: func$308
unused: func$309
unused: func$310
unused: func$311
unused: func$312
unused: func$313
unused: func$314
unused: func$315
unused: func$316
unused: func$317
unused: func$318
unused: func$319
unused: func$320
unused: func$321
unused: func$322
unused: func$323
unused: func$324
unused: func$325
unused: func$326
unused: func$327
unused: func$328
unused: func$329
unused: func$330
unused: func$331
unused: func$332
unused: func$333
unused: func$334
unused: func$335
unused: func$336
unused: func$337
unused: func$338
unused: func$339
unused: func$340
unused: func$341
unused: func$342
unused: func$343
unused: func$344
unused: func$345
unused: func$346
unused: func$347
unused: func$348
unused: func$349
unused: func$350
unused: func$351
unused: func$352
unused: func$353
unused: func$354
unused: func$355
unused: func$356
unused: func$357
unused: func$358
unused: func$359
unused: func$360
unused: func$361
unused: func$362
unused: func$363
unused: func$364
unused: func$365
unused: func$366
unused: func$367
unused: func$368
unused: func$369
unused: func$370
unused: func$371
unused: func$372
unused: func$373
unused: func$374
unused: func$375
unused: func$376
unused: func$377
unused: func$378
unused: func$379
unused: func$380
unused: func$381
unused: func$382
unused: func$383
unused: func$384
unused: func$385
unused: func$386
unused: func$387
unused: func$388
unused: func$389
unused: func$390
unused: func$391
unused: func$392
unused: func$393
unused: func$394
unused: func$395
unused: func$396
unused: func$397
unused: func$398
unused: func$399
unused: func$400
unused: func$401
unused: func$402
unused: func$403
unused: func$404
unused: func$405
unused: func$406
unused: func$407
unused: func$408
unused: func$409
unused: func$41
unused: func$410
unused: func$411
unused: func$412
unused: func$413
unused: func$414
unused: func$415
unused: func$417
unused: func$418
unused: func$419
unused: func$420
unused: func$421
unused: func$422
unused: func$423
unused: func$424
unused: func$425
unused: func$426
unused: func$427
unused: func$428
unused: func$429
unused: func$430
unused: func$431
unused: func$432
unused: func$433
unused: func$434
unused: func$435
unused: func$436
unused: func$437
unused: func$438
unused: func$439
unused: func$440
unused: func$441
unused: func$442
unused: func$443
unused: func$444
unused: func$445
unused: func$446
unused: func$447
unused: func$448
unused: func$449
unused: func$450
unused: func$451
unused: func$452
unused: func$453
unused: func$454
unused: func$455
unused: func$456
unused: func$457
unused: func$458
unused: func$459
unused: func$460
unused: func$461
unused: func$462
unused: func$463
unused: func$464
unused: func$465
unused: func$466
unused: func$467
unused: func$468
unused: func$469
unused: func$470
unused: func$471
unused: func$472
unused: func$473
unused: func$474
unused: func$475
unused: func$476
unused: func$477
unused: func$478
unused: func$52
unused: func$54
unused: func$55
unused: func$56
unused: func$57
unused: func$58
unused: func$59
unused: func$6
unused: func$60
unused: func$61
unused: func$62
unused: func$99