dmd
dmd copied to clipboard
dlang
add mangling for variadic parameter attributes for Issue 20818
I'll fix the spec and demangle.d in Phobos once this is approved.
Next step for https://issues.dlang.org/show_bug.cgi?id=20818
Thanks for your pull request, @WalterBright!
Bugzilla references
Your PR doesn't reference any Bugzilla issue.
If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
Testing this PR locally
If you don't have a local development environment setup, you can use Digger to test this PR:
dub run digger -- build "master + dmd#11128"
CC @ibuclaw @rainers
@WalterBright : Do you have any other mangling changes coming up ? Asking because @ibuclaw has submitted patches to libiberty (used by GDB & other coreutils) and I was recently looking into lldb support.
Could this be slightly ambiguous? Or at least, it would initially seem that there'll be a need to do a lookahead in order to determine whether M/Nk is for a scope T param or a scope ...
It's not ambiguous, and a lookahead isn't needed. You just read attributes until hitting Y or not.
It's not ambiguous, and a lookahead isn't needed. You just read attributes until hitting Y or not.
Not only attributes, but both attributes and parameter types.
Have I grok the proposed grammar change correctly?
TypeFunctionNoReturn:
CallConvention FuncAttrsopt Parameters(opt) ParamClose
Parameters:
Parameter
Parameter Parameters
Parameter:
Parameter2
M Parameter2 // scope
Nk Parameter2 // return
Parameter2:
Type
J Type // out
K Type // ref
L Type // lazy
ParamClose
X // variadic T t...) style
Y // variadic T t,...) style
Z // not variadic
If M/Nk is added to ParamClose, then it will collide with the definition for Parameter.
Not really, it won't match with Parameter because the "Type" will be a Y which is not a Type.
"Type" will be a Y which is not a Type.
According to the grammar Y can start a type as it can be an extern(Objective-C) mangled function. Semantically this cannot happen here AFAICT as you can only pass a pointer to a function as an argument, i.e. it starts with PY. But these special cases makes it pretty ugly to implement proper demangling and difficult to verify the grammar is unambiguous.
pretty ugly to implement proper demangling
It shouldn't be. Just test for the Y case first, if it isn't, then do what is done now. In any case, I said I'd implement it in Phobos.
@cybershadow the DAutoTest is failing with an error message that gives no clue?
It shouldn't be. Just test for the Y case first, if it isn't, then do what is done now. In any case, I said I'd implement it in Phobos.
You mean druntime ? ;)
And it's not only about core.demangle. There is a D demangler in libiberty (for gdb and coreutils), which is written in pure C. And hopefully one day there'll be one in LLVM too.
@CyberShadow the DAutoTest is failing with an error message that gives no clue?
Networking issue. If you look towards the end, you have this massive wall of timings, which is the output of wget (or curl) and you can ~clearly~ see that the download is way too slow and times out. We're seeing it in other PRs as well.
And it's not only about core.demangle. There is a D demangler in libiberty (for gdb and coreutils), which is written in pure C. And hopefully one day there'll be one in LLVM too.
Just copy what I write in core.demangle, then.
Networking issue. If you look towards the end, you have this massive wall of timings, which is the output of wget (or curl) and you can clearly see that the download is way too slow and times out. We're seeing it in other PRs as well.
Thank you!
One thing painfully missing from the log files is any exposition about what it is doing. It needs to use the echo command to insert a description of what the next thing it is trying to do is.
Every time I ask what is going on with an error message, it is an opportunity to improve the message, instead of me having to ask again and again. We don't accept this situation for compiler error messages, we shouldn't accept them in the log files, either. A few minutes by someone who knows what's going on with these tests will save a lot of frustration for me and everyone else who is faced with those log files, and those who are kind enough to answer our questions.
We have a bison implementation of the abi spec somewhere iirc, I'll see if I can find it.
We have a bison implementation of the abi spec somewhere iirc, I'll see if I can find it.
There are a couple of versions here: https://gist.github.com/rainers/6cdf73b48837defb9f88 showing different attempts to remove the ambiguities from the grammar adding additional constraints.
Not really, it won't match with Parameter because the "Type" will be a Y which is not a Type.
But you don't know whether it is really a Type or not until you reach the end, at which point, you are already midway through decoding TypeModifiers for a parameter where you usually expect a type to follow.
I think there really should be a marker at the beginning make it distinct from a normal parameter Type.
We have a bison implementation of the abi spec somewhere iirc, I'll see if I can find it.
There are a couple of versions here: https://gist.github.com/rainers/6cdf73b48837defb9f88 showing different attempts to remove the ambiguities from the grammar adding additional constraints.
Thanks. I'm probably a bit rusty with bison, though no matter which way I add it, I always got:
warning: rule useless in parser due to conflicts [-Wother]
| CallConvention Parameters ParamClose Type
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
So ended up with something like:
TypeFunctionNoReturn:
CallConvention FuncAttrsopt Parameters(opt) ParamClose
CallConvention FuncAttrsopt Parameters(opt) VariadicAttrs Y
VariadicAttrs:
TypeModifiers
M TypeModifiers
Nk TypeModifiers
Which adds only several more conflicts to your existing 53.
Though in reality, it will really look something like:
Parameter2:
TypeOrVariadicAttribs
J Type
K Type
L Type
TypeOrVariadicAttribs:
TypeModifiers
TypeModifiers TypeX
TypeX
TypeBackRef
So a new function will be required which initially starts off decoding TypeModifiers, but if it hits a Y, then will have to return immediately, otherwise continue parsing it as a Type.
But still, there's a lot of juggling that needs to be done, as Variadic parameter attributes will demangle differently to types with attributes. i.e: scope immutable(int*) vs. scope immutable ....
Which adds only several more conflicts to your existing 53.
I guess you are using one of the website-grammars. These don't work reasonably well with LALR(1). IIRC backref6.bison is the latest version that works without conflicts with some additional pseudo tokens that can be realized with some dynamic look-ahead of the 'lexer'.
Adding the modifications to ParamClose should be slightly better, but just adding 'M' already reports a shift/reduce conflict. I don't think this can easily work. Maybe some other character from the CallingConvention list can be used, with modifiers following and a terminating character.
Maybe some other character from the CallingConvention list can be used, with modifiers following and a terminating character.
I did use one initially, but took it out to make it shorter. Does it have to be LALR(1)? I don't see why.
I did use one initially, but took it out to make it shorter. Does it have to be LALR(1)? I don't see why.
It doesn't have to, but it makes it easier to implement and verify its sanity mechanically.
IIRC backref6.bison is the latest version that works without conflicts
Well, it does so only because the mangling of extern(Pascal) and extern(Objective-C) had been replaced with some proposal I made a couple of years ago. Removing the Pascal rule and using 'Y' for extern(Objective-C) yields one shift/reduce conflict. That can be solved by introducing a new token Yclose to be used instead of 'Y' while parsing the start of a parameter (assuming a parameter cannot start with a calling convention).
https://gist.githubusercontent.com/rainers/6cdf73b48837defb9f88/raw/c948888769fc9c83a82eaa5b1069321d090fd303/dmangle.bison shows a grammar with the scope/return attributes added to ParamClose without conflicts. So the addition seems ok.