Skip to content
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

cmd/compile: cleanup compiler flag processing #28570

Open
griesemer opened this Issue Nov 2, 2018 · 22 comments

Comments

Projects
None yet
2 participants
@griesemer
Copy link
Contributor

griesemer commented Nov 2, 2018

There are (at least) 3 ways in which compiler flags are internally specified and processed. It's a mess. Should clean up and provide a more systematic way of dealing with flags.

@griesemer griesemer added this to the Go1.13 milestone Nov 2, 2018

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Dec 15, 2018

@griesemer Hi. I'm happy to take that on. I'll check out the code and see how and where I could improve upon it. :)

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Dec 15, 2018

@Skarlso Once you get to it, before you make changes throughout, please send us an outline of your proposed changes first so we can provide feedback early on and make sure we're all on the same page. Thanks!

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Dec 16, 2018

@griesemer absolutely! Thanks! 🙂

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Dec 17, 2018

@griesemer Hi! So, I've been looking at the code, I'm not sure where to begin with this. Would you be so kind as to point me into the right direction from where I can then continue? :) Thanks 👍.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Dec 17, 2018

I was looking at these: https://github.com/golang/go/blob/master/src/cmd/compile/internal/gc/main.go#L186
Am I in the right direction?

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Dec 17, 2018

@Skarlso Yes, that's one of the many ways we set flags. Ideally we'd just use the flags package, but that may not be possible easily. Before writing/changing code, we need to collect all the flag variants used in the code in a spreadsheet or text document (Google Sheets, Docs) so that we can get an overview of what's there, and what's needed. With that information in hand it should be possible to design a replacement mechanism that ideally encompasses all the existing mechanisms in a more uniform way. After that, the code changes should be straight-forward. That is, this is a bit of design exercise.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Dec 18, 2018

Great! :) I'm even more excited to do this now! I'll open up a spreadsheet for it. Thanks again!

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Dec 18, 2018

@griesemer https://docs.google.com/spreadsheets/d/15GFcr46L01Shh2Oag2ANLkfBB5SMnF5YQvFxmeU2n-M/edit#gid=0 Is this going into the right direction?

I started to learn and read what's happening here and take notes into my trusty moleskin. I'll whip something up once I finish that list there.

Some of this I need to understand first in order to extract some valuable info.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Dec 18, 2018

@Skarlso This looks reasonable. I've commented on the spreadsheet.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Dec 22, 2018

@griesemer Hi! I finished the sheet of flags. :) That was quite something. I should mention that these are the obvious flags. None of the conditionals and things like these: Ctxt.Debugasm = Debug['S'].

Which could be considered flags. Am I to include those as well?

In essense:

It looks like everything that is not using flags is actually using flags in the background. Everything that is objabi is using flag.Var in the background. It's just a convenient wrapper around it. I'll have to ponder this, but it looks like we might be able to get away with just using flags.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Dec 22, 2018

@Skarlso Thanks for doing this; that's going to be very helpful moving forward. We'll have a closer look but probably after the xmas break.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Jan 19, 2019

@griesemer hi! Can I move forward in this or did you want to take a look at the sheet before I proceed? 🙂

Also, happy news year. 😀

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Jan 19, 2019

@Skarlso Sorry, I've been busy with the Go 1.13 changes. You're welcome to move forward, but before you do, and now that we have all the information, what's your plan? That is, how are you planning to unify the various flags into a more regular pattern? We should first agree on that. Afterwards, making the actual change shouldn't be too hard. Coming up with the concrete plan may require some experimentation, though.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Jan 21, 2019

@griesemer Hi! Ahh, sorry. I don't have a plan as of today, as I was kind of waiting for you to make a move. I thought you wanted to take a look first. :) My bad.

I have an outline of a plan. I thought that I'd unify them all to use the objapi since that's the most used and it actually uses flags in the background. That's my first step. To put them under a common denominator. After that I can see better what's up with that Debug['d'] slice for example. Most of those are actually only ever used once and as a counter maybe.

So, first step, common denominator, second step refactor flag usage / declaration.

What do you think?

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Jan 23, 2019

@Skarlso Sorry for the delay, I haven't had any time to focus on this yet as I'm still busy with Go 1.13 changes. I'll try to get to this soon.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Jan 23, 2019

@Skarlso Sorry for the delay, I haven't had any time to focus on this yet as I'm still busy with Go 1.13 changes. I'll try to get to this soon.

No problem! Thanks very much. :)

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Feb 27, 2019

@griesemer Hiiiiiiii.... :))))

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Feb 27, 2019

Yes, I am aware of this. It's still on my list.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Mar 5, 2019

@Skarlso Hello there. Apologies for the delays, and thanks again for the spreadsheet. I spent some time studying it a bit. I'm not convinced that we should be using objabi flags - those are there for historic reasons, but all newer code uses the flag package directly. objabi also uses the flag package since that is the package that takes care of the parsing. It would be good to know what else is using objabi flags(the linker? assembler?). And what functionality of those flags is crucial. Maybe some of it is used but we could get rid of it anyway (by changing how the respective flags are set).

So, can one get away with just the flag package? I don't know. There is the objabi.FlagCount mechanism which permits increasing a value by providing a flag multiple times (-v -v) I think, but I don't know where this is used. We should be able to get rid of the Debug[ch] flags though by giving them proper internal names.

Also, all these flags are global variables, and it's not always clear where they are coming from. It might be nice to organize and name them better.

I envision somethings like a flags.go file which organizes this cleanly. All the flags are defined in there. To make it clear that they are flags in the rest of the code, they could be grouped into a struct:

var flags struct {
   // customization
   noBoundChecking bool
   noColumnNumbers bool
   localImportPath string
   ...

   // debug options
   haltOnError bool
   ...

   // etc.
}

Code that uses the flags would write flags.haltOnError or the like. This would make it clear that we're talking about flags.

It's probably too hard to do this all in one go. One could perhaps start with a small number of flags, 10 or 20, maybe of a couple of different categories, as proof of concept. Starting with a small, clean CL, that outlines the basic idea. It will take several rounds to get this right and involve trial and error. It's not just doing the coding but also organizing the flags better, document them, etc.

I'm afraid I won't be able to guide you through each step w/o basically doing the step (and the research) myself; this is really a low-priority cleanup job and we have a lot of other, more important stuff on our plates at the moment.

I do think the spreadsheet is very useful though, to get started, so thanks again for that. If you want to keep working on this, please feel free. I'd start with some actual experiments in code to see where it leads to, perhaps with small CLs for feedback.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Mar 5, 2019

I should add (and I should have said that early on), that these kinds of refactoring (such as this flag cleanup) take a bit of experience and "Fingerspitzengefühl" as one would say in German, and some familiarity with the code base. So this is not good starter project. Just something you may want to keep in mind. Thanks.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Mar 5, 2019

Oh man, Fingerspitzengefühl is a word I haven't heard in a long time! Thanks for the nostalgia.

I do have experience with refactoring large chunks of code so hopefully I can come up with a POC that has a minimal working version and a very clean CL.

Thanks for summing it. I do would like to do this still. 😊🙂👍

@griesemer

This comment has been minimized.

Copy link
Contributor Author

griesemer commented Mar 5, 2019

@Skarlso Ok, sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.