sifive / wake

The SiFive wake build tool
Other
86 stars 28 forks source link

Wake core dumps on the string `"%"` #1419

Open bmitc opened 1 year ago

bmitc commented 1 year ago

This was happened upon while trying to get "%" as a verbatim string in Wake. (Note, the best way I found was integerToUnicode 37.) While experimenting with various methods, I happened to try:

$ wake -x '"%"'
<command-line>:1:[9-11]: syntax error; found illegal token '"%"', but handling it like '"%'
<command-line>:1:12: syntax error; found illegal token '', but handling it like '%"'
wake: src/util/rank.cpp:266: uint32_t RankSelect1Map::select1(uint32_t) const: Assertion `rank1 < num1s' failed.
Aborted (core dumped)

and got the core dump.

This is not a critical issue since it's easily worked around by using integerToUnicode, but I wanted to file this just to capture the find.


As a secondary issue, should "%" properly resolve to a string? I think right now, it's getting interpreted as a half-complete multi-line string "%...%". Maybe when that multi-line string syntax is removed in favor of """...""" we can test to make sure "%" can be input as or inside a normal string?

V-FEXrt commented 11 months ago

Okay, I see what's going on here.

This is a problem that will just go away once we remove legacy multiline strings. The first part "% is being processed as the start to a multiline string and the remaining " without a %" is confusing the parser. The crash only happens when calling -x directly because that handles expressions differently. If you do it inside of a file you get a proper parse error without a crash.

For now I'll leave this open but going to wait for legacy string removal to actually do anything here.

For completeness, here is another way to get the string literal "%"

$ cat do.wake 
package foo

export def x _ = """
    %
    """
$ wake --in foo x
"%"
bmitc commented 11 months ago

For now I'll leave this open but going to wait for legacy string removal to actually do anything here.

That sounds good to me. This is for sure an edge of edge case, especially after your investigation. Thanks for taking a look at this one!