Hi Ian,
Thanks for replying! (Also I'm really sorry for the missing hard wrapping
in my earlier email, I shouldn't have sent that from my phone)
Regarding this being a blocker: we've already made the changes to Rust, and
the new ("v0") mangling format continues to remain opt-in and nightly-only,
as long as no distributions ship gdb & other tools, capable of demangling it.
I agree that landing the extra changes later on top should be fine anyway,
though when I make the sprintf -> snprintf change, I could provide the extra
changes as well (either as a combined patch, or as a second tiny patch).
Sadly I don't think I can get to either until next week, hope that's okay.
For correctness, I've been running this test after every change I make, both
with the standalone copy (in the same gist), and a local build of libiberty:
https://gist.github.com/eddyb/c41a69378750a433767cf53fe2316768#file-test-rs-L85-L116
The input is almost 1 million symbol names (collected from a full build of
the Rust compiler and Cargo), and the test ensures that the C implementation
produces the same result as the original "v0" demangler (written in Rust).
All that data is around 1GB but maybe I should try to upload it somewhere
public so that anyone can repeat this procedure, or I could even redo the
collection process (in order to gather even more / newer symbol names).
While this may not stress every aspect of the demangler, it is comprehensive
enough that it found several kinds of bugs in a 3rd party implementation
(which the IllumOS project wrote from scratch, instead of porting ours).
Thanks,
- Eddy B.
On Thu, Oct 29, 2020, at 00:45, Ian Lance Taylor wrote:
> On Wed, Oct 28, 2020 at 3:22 PM Nikhil Benesch via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > I think it is mostly a matter of snagging some of Ian's limited time.
> > I suspect it is still worthwhile to try to get the original patch
> > reviewed and merged, because then any follow-up changes for const
> > generics support will be smaller and easier to review.
>
> Yeah, it's not a good idea for me to be a blocker for changes to Rust code.
>
> I took a quick look at the original patch. The calls to sprintf
> should use snprintf instead.
>
> Other than that it seems fine though I have no idea whether it's correct.
>
> Ian
>
>
>
> > On Wed, Oct 28, 2020 at 5:48 PM Eduard-Mihai Burtescu <ed...@lyken.rs>
> > wrote:
> > >
> > > FWIW, the patch has become slightly outdated compared to the Rust
> > > upstream, so if someone wants to review it I should prepare a new version.
> > >
> > > The changes would be for the MVP version of "const generics" (Rust's
> > > equivalent to C++ templates with value parameters, enabling e.g. types
> > > like `CustomArray<T, 123>`), which supports a few different kinds of
> > > primitive values, whereas the original patch only handled unsigned
> > > integers.
> > >
> > > Thanks,
> > > - Eddy B.
> > >
> > > On Wed, Oct 28, 2020, at 23:25, Nikhil Benesch wrote:
> > > > On 10/28/20 5:22 PM, Nikhil Benesch wrote:
> > > > > Ian, are you able to review this? I saw that you reviewed many of the
> > > > > prior changes to the Rust demangler.
> > > > >
> > > > > If not, can you suggest someone who can?
> > > > >
> > > > > Thanks very much.
> > > > > Nikhil
> > > >
> > > > I seem to have failed to convince my email client to set the appropriate
> > > > reply to headers. This is the patch to which I was referring:
> > > >
> > > > https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542488.html
> > > >
>
