Re: Rust front-end

[David Malcolm via Gcc-rust]

On Tue, 2022-10-04 at 13:29 +0100, Philip Herron wrote:
> Hi everyone,
> 
> As the cut-off for merging is coming up in November, quite a few of
> our patches have not been reviewed yet.
> 
> There are a few main issues that have been raised so far, and we are
> fixing those at the moment in preparation for version 3 of the
> patches. Is there anything else we can do to make reviewing the rest
> of the patches easier?

Do you have a list of which patches need reviewing?
e.g. perhaps a page showing:
- which patches are waiting for a reviewer, as opposed to
- which patches are already approved
- which patches have issues identified in review
  - ...where no-one is yet working on addressing them
  - ...where someone is working on addressing them
etc

to make it clearer what the next action is for each patch, and who is
meant to be taking it.

(within Red Hat, we used to call this "who has the ball?")

Hope this is constructive
Dave

> 
> Thanks
> 
> --Phil
> 
> On Mon, 11 Jul 2022 at 16:02, David Edelsohn 
> wrote:
> > 
> > On Mon, Jun 27, 2022 at 10:52 AM Philip Herron
> >  wrote:
> > > 
> > > Hi everyone,
> > > 
> > > Since November 2020, I've worked full-time on the Rust front-end
> > > for
> > > GCC, thanks to Open Source Security, Inc and Embecosm. As a
> > > result, I
> > > am writing to this mailing list to seek feedback from the
> > > collective
> > > experience here early to plan a path for upstreaming the front-
> > > end
> > > into GCC.
> > > 
> > > 1. What is the actual process of merging a prominent feature like
> > > this upstream
> > >   - How do we review this?
> > >   - Do we create a "mega-commit" patch
> > >   - How long should we expect this review process to take
> > >   - Is there anything we can do to make this easier?
> > > 
> > > 2. What sort of quality does the GCC community expect?
> > >   - I think it is essential that we can compile valid test cases
> > > from
> > > a testsuite and real projects before merging.
> > >   - It seems reasonable that our error handling may not be 100%
> > > but be
> > > expected to improve over time
> > >   - Upon merging, can features like Rust be marked as
> > > experimental
> > > 
> > > 3. How do GCC releases work?
> > >   - If you miss a window can we still merge code into the front-
> > > end?
> > >   - Can we merge without a borrow checker and backport this in
> > > the future?
> > > 
> > > 4. What about the possibility of merging sooner rather than
> > > later,
> > > which would help the project gain interest through the increased
> > > visibility of it as part of the GCC family.
> > >   - Does this still allow for development churn, or will it cause
> > > friction?
> > > 
> > > 5. Does anyone have prior experience or advice they could give
> > > us?
> > > 
> > > For some context, my current project plan brings us to November
> > > 2022
> > > where we (unexpected events permitting) should be able to support
> > > valid Rust code targeting Rustc version ~1.40 and reuse libcore,
> > > liballoc and libstd. This date does not account for the borrow
> > > checker
> > > feature and the proc macro crate, which we have a plan to
> > > implement,
> > > but this will be a further six-month project.
> > > 
> > > Regarding patch management, we currently do our development on
> > > GitHub:
> > > https://github.com/Rust-GCC/gccrs; this means we can integrate
> > > our
> > > issue tracking with the official Rust project by linking back to
> > > the
> > > official Rust project's RFC issues, for example. The downside is
> > > that
> > > when someone uses our compiler and hits an ICE, they will be
> > > directed
> > > to the GCC Bugzilla, which is correct but can lead to a mismatch
> > > in
> > > issue tracking. Nevertheless, I think it's essential to have the
> > > GitHub link here to integrate with the broader Rust community. I
> > > believe we can triage Rust issues on the Bugzilla and raise
> > > associated
> > > ones on Github to manage this.
> > > 
> > > From my perspective as the lead on this front-end, we are
> > > currently
> > > under heavy development, so this means a fair amount of code
> > > churn
> > > still, and I don't see this changing until we can successfully
> > > compile
> > > the libcore crate later this year. Although I would love to see
> > > us
> > > merged into GCC 13, I want to make sure this project is a success
> > > for
> > > everyone, and this might mean pushing back to the next release
> > > window
> > > to make sure this is manageable to produce a quality front-end to
> > > sit
> > > alongside the others.
> > > 
> > > I wish to thank you those in the GCC developer community, who
> > > have
> > > inspired me and helped me navigate my journey to this point in
> > > time.
> > > 
> > > - Thomas Schwinge
> > > - Mark Wielaard
> > > - Tom Tromey
> > > - Ian Lance Taylor
> > > - David Edelsohn
> > > - David Malcolm
> > > - Martin Jambor
> > 
> > Congratulations! The GCC Steering Committee has voted to accept the
> > contribution of the Ru

Re: [PATCH Rust front-end v3 20/46] gccrs: Add wrapper for make_unique

[David Malcolm via Gcc-rust]

On Wed, 2022-10-26 at 10:17 +0200, arthur.co...@embecosm.com wrote:
> From: Philip Herron 
> 
> This is a wrapper for make_unique we can likely get rid of this as
> there
> are other implementations available or simply keep using the
> unique_ptr
> constructor.

[CCing Jonathan]

As it happens, I just posted something similar here for review:
  [PATCH v3] Add gcc/make-unique.h
https://gcc.gnu.org/pipermail/gcc-patches/2022-October/604442.html
which I hope to use in many places in the analyzer.

Dave

> ---
>  gcc/rust/util/rust-make-unique.h | 35
> 
>  1 file changed, 35 insertions(+)
>  create mode 100644 gcc/rust/util/rust-make-unique.h
> 
> diff --git a/gcc/rust/util/rust-make-unique.h b/gcc/rust/util/rust-
> make-unique.h
> new file mode 100644
> index 000..7b79e625ff1
> --- /dev/null
> +++ b/gcc/rust/util/rust-make-unique.h
> @@ -0,0 +1,35 @@
> +// Copyright (C) 2020-2022 Free Software Foundation, Inc.
> +
> +// This file is part of GCC.
> +
> +// GCC is free software; you can redistribute it and/or modify it
> under
> +// the terms of the GNU General Public License as published by the
> Free
> +// Software Foundation; either version 3, or (at your option) any
> later
> +// version.
> +
> +// GCC is distributed in the hope that it will be useful, but
> WITHOUT ANY
> +// WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +// FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License
> +// for more details.
> +
> +// You should have received a copy of the GNU General Public License
> +// along with GCC; see the file COPYING3.  If not see
> +// .
> +
> +#ifndef RUST_MAKE_UNIQUE_H
> +#define RUST_MAKE_UNIQUE_H
> +
> +#include "rust-system.h"
> +
> +namespace Rust {
> +
> +template 
> +std::unique_ptr
> +make_unique (Ts &&...params)
> +{
> +  return std::unique_ptr (new T (std::forward (params)...));
> +}
> +
> +} // namespace Rust
> +
> +#endif // RUST_MAKE_UNIQUE_H

-- 
Gcc-rust mailing list
Gcc-rust@gcc.gnu.org
https://gcc.gnu.org/mailman/listinfo/gcc-rust

Re: [PATCH Rust front-end v3 35/46] gccrs: Add metadata ouptput pass

[David Malcolm via Gcc-rust]

%{On Wed, 2022-10-26 at 10:18 +0200, arthur.co...@embecosm.com wrote:
> From: Philip Herron 
> 
> Extern crates statements to tell the front-end to look for another
> library.
> The mechanism here is heavily inspired from gccgo, so when we compile
> a
> library for example we invoke:
> 

[...snip...]

> +  rust_error_at (Location (),
> +    "expected metadata-output path to have base file
> name of: "
> +    "%<%s%> got %<%s%>",
> +    expected_file_name.c_str (), path_base_name);

I can't comment on the patch in depth, but does rust_error_at call into
GCC's regular diagnostics?

If so, "%qs" is a more idiomatic way to express printing a string
argument in quotes (and bold), rather than "%<%s%>", though IIRC they
do the same thing (unless I'm missing something?).

This shows up in a few places in this patch, and might affect other
patches in the kit - though it's a minor nitpick, of course.

Dave

-- 
Gcc-rust mailing list
Gcc-rust@gcc.gnu.org
https://gcc.gnu.org/mailman/listinfo/gcc-rust

Re: Rust frontend patches v3

[David Malcolm via Gcc-rust]

On Wed, 2022-10-26 at 10:17 +0200, arthur.co...@embecosm.com wrote:
> This is the fixed version of our previous patch set for gccrs - We've
> adressed
> the comments raised in our previous emails.

[...snip...]

(Caveat: I'm not a global reviewer)

Sorry if this is answered in the docs in the patch kit, but a high-
level question: what's the interaction between gccrs and gcc's garbage
collector?  Are the only GC-managed objects (such as trees) either (a)
created near the end of the gccrs, or (b) common globals created at
initialization and with GTY roots?  Are there any points where a
collection happen within gccrs?  Or is almost everything stored using
gccrs's own data structures, and are these managed in the regular (non-
GC) heap?

I skimmed the patches and see that gccrs uses e.g. std::vector,
std::unique_ptr, std::map, and std::string; this seems reasonable to
me, but it got me thinking about memory management strategies.

I see various std::map e.g. in Rust::Compile::Context; so e.g.
is the GC guaranteed never to collect whilst this is live?

Hope this is constructive
Dave

-- 
Gcc-rust mailing list
Gcc-rust@gcc.gnu.org
https://gcc.gnu.org/mailman/listinfo/gcc-rust

Re: Rust frontend patches v3

[David Malcolm via Gcc-rust]

On Fri, 2022-10-28 at 13:48 +0200, Arthur Cohen wrote:
> Hi David,
> 
> On 10/26/22 23:15, David Malcolm wrote:
> > On Wed, 2022-10-26 at 10:17 +0200, arthur.co...@embecosm.com wrote:
> > > This is the fixed version of our previous patch set for gccrs -
> > > We've
> > > adressed
> > > the comments raised in our previous emails.
> > 
> > [...snip...]
> > 
> > (Caveat: I'm not a global reviewer)
> > 
> > Sorry if this is answered in the docs in the patch kit, but a high-
> > level question: what's the interaction between gccrs and gcc's
> > garbage
> > collector?  Are the only GC-managed objects (such as trees) either
> > (a)
> > created near the end of the gccrs, or (b) common globals created at
> > initialization and with GTY roots? 
> 
> We only create trees at the last point of our compilation pipeline, 
> before directly writing them to the backend. This then calls a 
> `write_global_definitions` method, that we ported over directly from
> the 
> Go frontend. Among other things, this method has the role of
> preserving 
> trees from the GC using `go_preserve_from_gc()` (or 
> `rust_preserve_from_gc()` in our case).
> 
> Elsewhere in our pipeline, we never call any garbage-collection
> routines 
> or GC-related functions.
> 
> > Are there any points where a collection happen within gccrs?  Or is
> > almost everything stored using
> > gccrs's own data structures, and are these managed in the regular
> > (non-
> > GC) heap?
> 
> This is correct. We have an AST representation, implemented using
> unique 
> pointers, which is then lowered to an HIR, also using unique
> pointers.
> 
> > I skimmed the patches and see that gccrs uses e.g. std::vector,
> > std::unique_ptr, std::map, and std::string; this seems reasonable
> > to
> > me, but it got me thinking about memory management strategies.
> > 
> > I see various std::map e.g. in Rust::Compile::Context; so
> > e.g.
> > is the GC guaranteed never to collect whilst this is live?
> 
> This is a really interesting question, and I hope the answer is yes!
> But 
> I'm unsure as to how to enforce that, as I am not too familiar with
> the 
> GCC GC. I'm hoping someone else will weigh in. As I said, we do not
> do 
> anything particular with the GC during the execution of our 
> `CompileCrate` visitor, so hopefully it shouldn't run.

I'm guessing that almost all of gccrs testing so far has been on
relatively small examples, so that even if the GC considers collecting,
the memory usage might not have exceeded the threshold for actually
doing the mark-and-sweep collection, and so no collection has been
happening during your testing.

In case you haven't tried yet, you might want to try adding:
  --param=ggc-min-expand=0 --param=ggc-min-heapsize=0
which IIRC forces the GC to actually do its mark-and-sweep collection
at every potential point where it might collect.

I use these params in libgccjit's test suite; it massively slows things
down, but it makes any GC misuse crash immediately even on minimal test
cases, rather than hiding problems until you have a big (and thus
nasty) test case.

Hope this is helpful
Dave


> 
> > Hope this is constructive
> > Dave
> > 
> 
> Thanks a lot for the input,
> 
> All the best,
> 
> Arthur
> 
> 
> 
> 

-- 
Gcc-rust mailing list
Gcc-rust@gcc.gnu.org
https://gcc.gnu.org/mailman/listinfo/gcc-rust

Re: Rust frontend patches v3

[David Malcolm via Gcc-rust]

On Fri, 2022-10-28 at 17:20 +0200, Arthur Cohen wrote:
> 
> 
> On 10/28/22 15:06, David Malcolm wrote:
> > On Fri, 2022-10-28 at 13:48 +0200, Arthur Cohen wrote:
> > > Hi David,
> > > 
> > > On 10/26/22 23:15, David Malcolm wrote:
> > > > On Wed, 2022-10-26 at 10:17 +0200,
> > > > arthur.co...@embecosm.com wrote:
> > > > > This is the fixed version of our previous patch set for gccrs
> > > > > -
> > > > > We've
> > > > > adressed
> > > > > the comments raised in our previous emails.

[...snip...]

> > 
> > I'm guessing that almost all of gccrs testing so far has been on
> > relatively small examples, so that even if the GC considers
> > collecting,
> > the memory usage might not have exceeded the threshold for actually
> > doing the mark-and-sweep collection, and so no collection has been
> > happening during your testing.
> > 
> > In case you haven't tried yet, you might want to try adding:
> >    --param=ggc-min-expand=0 --param=ggc-min-heapsize=0
> > which IIRC forces the GC to actually do its mark-and-sweep
> > collection
> > at every potential point where it might collect.
> 
> That's very helpful, thanks a lot. I've ran our testsuite with these
> and 
> found no issues, but we might consider adding that to our CI setup to
> make sure.

Great!   Though as noted, for libgccjit it slows the testsuite down
*massively*, so you might want to bear that in mind.  I'm doing it for
libgccjit because libgccjit looks like a "frontend" to the rest of the
GCC codebase, but it's a deeply weird one, and so tends to uncover
weird issues :-/

Dave

> 
> Kindly,
> 
> Arthur
> 
> > I use these params in libgccjit's test suite; it massively slows
> > things
> > down, but it makes any GC misuse crash immediately even on minimal
> > test
> > cases, rather than hiding problems until you have a big (and thus
> > nasty) test case.
> > 
> > Hope this is helpful
> > Dave
> > 
> > 
> > > 
> > > > Hope this is constructive
> > > > Dave
> > > > 
> > > 
> > > Thanks a lot for the input,
> > > 
> > > All the best,
> > > 
> > > Arthur
> > > 
> > > 
> > > 
> > > 
> > 

-- 
Gcc-rust mailing list
Gcc-rust@gcc.gnu.org
https://gcc.gnu.org/mailman/listinfo/gcc-rust

Re: [PATCH 1/2] diagnostics: add error_meta

[David Malcolm via Gcc-rust]

On Wed, 2023-09-06 at 15:53 +0200, Arthur Cohen wrote:
> From: David Malcolm 

I guess I can review this patch :)

Needs a ChangeLog entry, so here's one:

gcc/ChangeLog
* diagnostic-core.h (error_meta): New decl.
* diagnostic.cc (error_meta): New.

Also, needs a signed-off-by, so here's one:

Signed-off-by: David Malcolm 


OK for trunk with those added.

Thanks
Dave



> 
> ---
>  gcc/diagnostic-core.h |  3 +++
>  gcc/diagnostic.cc | 15 +++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/gcc/diagnostic-core.h b/gcc/diagnostic-core.h
> index 7334c79e8e6..c9e27fd2e6e 100644
> --- a/gcc/diagnostic-core.h
> +++ b/gcc/diagnostic-core.h
> @@ -92,6 +92,9 @@ extern void error_n (location_t, unsigned
> HOST_WIDE_INT, const char *,
>  extern void error_at (location_t, const char *, ...)
> ATTRIBUTE_GCC_DIAG(2,3);
>  extern void error_at (rich_location *, const char *, ...)
>    ATTRIBUTE_GCC_DIAG(2,3);
> +extern void error_meta (rich_location *, const diagnostic_metadata
> &,
> +   const char *, ...)
> +  ATTRIBUTE_GCC_DIAG(3,4);
>  extern void fatal_error (location_t, const char *, ...)
> ATTRIBUTE_GCC_DIAG(2,3)
>   ATTRIBUTE_NORETURN;
>  /* Pass one of the OPT_W* from options.h as the second parameter. 
> */
> diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
> index c523f215bae..65c0cfbf11a 100644
> --- a/gcc/diagnostic.cc
> +++ b/gcc/diagnostic.cc
> @@ -2108,6 +2108,21 @@ error_at (rich_location *richloc, const char
> *gmsgid, ...)
>    va_end (ap);
>  }
>  
> +/* Same as above, but with metadata.  */
> +
> +void
> +error_meta (rich_location *richloc, const diagnostic_metadata
> &metadata,
> +   const char *gmsgid, ...)
> +{
> +  gcc_assert (richloc);
> +
> +  auto_diagnostic_group d;
> +  va_list ap;
> +  va_start (ap, gmsgid);
> +  diagnostic_impl (richloc, &metadata, -1, gmsgid, &ap, DK_ERROR);
> +  va_end (ap);
> +}
> +
>  /* "Sorry, not implemented."  Use for a language feature which is
>     required by the relevant specification but not implemented by
> GCC.
>     An object file will not be produced.  */

-- 
Gcc-rust mailing list
Gcc-rust@gcc.gnu.org
https://gcc.gnu.org/mailman/listinfo/gcc-rust

Re: [PATCH 2/2] Experiment with adding an error code to an error

[David Malcolm via Gcc-rust]

On Wed, 2023-09-06 at 15:53 +0200, Arthur Cohen wrote:
> From: David Malcolm 

This is probably something for the gcc-rust maintainers to review
(rather than me self-reviewing with my "diagnostics maintainer" hat
on).

Doesn't have a ChangeLog entry, FWIW.
Doesn't have a signed-off-by, so here's one:

Signed-off-by: David Malcolm 

[...snip...]

> diff --git a/gcc/rust/rust-gcc-diagnostics.cc b/gcc/rust/rust-gcc-
> diagnostics.cc
> index 72d2c068541..58c0a5654ea 100644
> --- a/gcc/rust/rust-gcc-diagnostics.cc
> +++ b/gcc/rust/rust-gcc-diagnostics.cc

[...snip...]

> +void
> +rust_be_error_at (const RichLocation &location, const ErrorCode
> code,
> + const std::string &errmsg)
> +{
> +  /* TODO: 'error_at' would like a non-'const' 'rich_location *'. 

The above comment should refer to "error_meta", rather than
"error_at"...

> */
> +  rich_location &gcc_loc = const_cast (location.get
> ());
> +  diagnostic_metadata m;
> +  rust_error_code_rule rule (code);
> +  m.add_rule (rule);
> +  error_meta (&gcc_loc, m, "%s", errmsg.c_str ());

... to match this call.

[...snip...]

Otherwise, LGTM, but as I said, this is more in the gcc-rust
maintainers' area.

Dave

-- 
Gcc-rust mailing list
Gcc-rust@gcc.gnu.org
https://gcc.gnu.org/mailman/listinfo/gcc-rust