cmd/compile: cleanup compiler flag processing

[griesemer]

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.

[Skarlso]

@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]

@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]

@griesemer absolutely! Thanks! 🙂

[Skarlso]

@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]

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]

@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]

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

[Skarlso]

@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]

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

[Skarlso]

@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]

@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]

@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]

@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]

@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]

@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]

@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]

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

[griesemer]

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

[griesemer]

@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]

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]

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]

@Skarlso Ok, sounds good.

[eduardbme]

Hi @griesemer @Skarlso Do you have a copy of the document mentioned above ? (provided link does not work)

[Skarlso]

I'll take a look in my sheet drive. I'm hoping there will be some kind of copy there.

[eduardbme]

Hello there @griesemer I came through the codebase, and here are my thoughts about this task.

1. objabi and objabi/flag.go Seems like objabi should not have a flag logic and a flags package declaration. As long as I understand, 'objabi' stands for a binary interface and thus the logic within this package should be specific to binary object files, not flags and specific flags specializations.

2. A set of flags specializations. Just to mention a few: MultiFlag, CountFlag, ternaryFlag, StringsFlag, explicitStringFlag, boolFlag, jsonFlag, PerPackageFlag, upgradeFlag, DebugFlag, etc. Some flag types have been reimplemented across the different packages. Seems like we can normalize these special cases, put them under the flag package, and reuse across the code base.

3. Some flags are used across different packages, and so, as you said earlier, we can place the most commonly used flags under the flags structure. I don’t think that it’s a good idea to put every flag in that structure (at least for now). Tests and one-time cases would still use the flag package, so it would be kind of a combination of the flag and flags usage. But at least we can consider the way how to manage flag duplicates and see whether it would be suitable.

So, here is the plan:

1. Replace objabi.AddVersionFlag with flag.HandleVersion (for example) and all calls over the places, there are quite a few of them.

2. Migrate all logic related to the flags and specialized flags from objabi/flag.go to the flag package. After this step, we would have a unified way to access flags and no calls to objabi.

3. Review all specialized flags across the codebase, remove duplications, and place them under the flag package. After this step, all logic related to the flags and special cases would be in one place under one package.

4. Review how many flag duplicates we have across the code and consider creating a flags structure with the most commonly used flags.

So, I think we should go with small steps, thus, this particular issue can be decomposed into smaller ones, following the plan.

[ianlancetaylor]

I'll just note that you can't add all this stuff to the publicly visible flag package, at least not without a set of proposals (https://go.dev/s/proposal), but those proposals won't be accepted. The flags got stuffed into cmd/internal/objabi as part of a cleanup of shared functionality of various commands. We can move the flags into something like cmd/internal/cmdflag, but we can't move them to flag, and we shouldn't move them to a package named "flag" as that would be confusing.

[eduardbme]

can't add all this stuff to the publicly visible flag package

Makes sense. Let's imaging we are talking about some internal package, we can go with a cmd/internal/cmdflag for now.

[Skarlso]

It's gone, I'm afraid. I have no idea how or why but most, if not all of my Google Sheets are gone. :/ Probably something went wrong when I migrated away from Drives. :/