Reject -o flag when using -verilog

[brooksbp]

Proposed fix for #47 .

@quark17 not sure this is the right solution (e.g. I intended for this to be a warning but it's acting like an error), but I thought I'd post it early and iterate on your feedback!

[nanavati]

Whether a flag is named as a warning or error Wxxx vs Exxx is just a convention. What it actually does depends on how the message fits into the control flow (and whether the user has promoted some warnings to errors and so on.

In this case, the flag handler either returns a function to update to update the current Flags data OR a message explaining why flag decoding failed (i.e. an error). That being said, I think an error totally fine here. If someone passes in the -o flag when the compiler isn't linking a simulation executable it's not going to do anything sensible with it. Failing is probably better than charging off and doing the wrong thing.

However, there is another subtlety here. The test isn't actually Bluesim vs Verilog, it's compiling [.bs, .bsv] -> [.ba, .v] vs linking [.ba, .v] -> an "executable" (often a shell script invoking Bluesim or a Verilog simulator). I think that means the check doesn't belong in flag decoding but rather in checkBSrcFlags vs checkLinkFlags

Does that make sense?

[quark17]

I also came here to apologize that the original issue is not well worded. As @nanavati said, the -o flag is very much allowed with -verilog when linking a simulation. The issue is that -o is not used when compiling BSV/BH source files, and this is true regardless of the target backend (either -verilog or -sim).

The decodeArgs function determines what mode the user has invoked BSC for and indicates this by returning one of the values of the type Decode. Most commonly, the user is either compiling source files (DBlueSrc) or they are linking one of the backends (DVerLink and DSimLink). Once BSC has decided which is intended, there are functions which further check that the user has provided flags that are self-consistent and appropriate for that mode.

The two modes can accept many of the same flags, but many other flags are specific to one or the other. For the most part, these aren't checked! I would guess that there are many more flags besides -o that BSC should warn or error about. I think this points to another wrinkle: I don't think we can afford to create a new error tag for every flag. You've created an error tag, WOFileWithVerilogBackend -- even if we renamed it to something like WOFlagWhenCompile, it would still be too much have a tag like that for every flag. I think we'd want to create an error like WIgnoredFlagForCompile (or EInvalidFlagForCompile), that took a String argument indicating the flag. This would mean that we'd also need an error WIgnoredFlagForLink. Maybe we want to merge those both into WIgnoredFlagForMode, that possibly takes another String naming the mode.

The decodeArgs function returns warnings separately from errors. You'll notice that its return type is ([WMsg], Decoded) where the first item in the pair is any warnings. This allows BSC to return a decoding (like DVerLink) while also returning some warnings to be printed. Errors are returned as their own decoding: DError.

At the moment, the only warnings that occur are warnings while parsing individual flags. You'll see, for example, that once decodeArgs has determined the mode is linking, it returns this:

(warnings, checkLinkFlags flags names)

This doesn't let the checkLinkFlags function contribute any warnings. It can only return DError. I believe that we'll want to change checkLinkFlags and checkBSrcFlags to return a pair, of the warnings and the decode result. (I don't know offhand if it's better to pass the existing warnings as an argument, so that the check functions can add warnings to it and return the final result; or whether the check functions should return just the new warnings and the caller combines them with the earlier warnings to return the final result.)

But I'm also OK with this being an error. (If we wanted it to be an error that can be downgraded, that would require some extra code in decodeArgs to adjust between the two.)

[brooksbp]

@nanavati @quark17 OK, this is making sense. If we check flag compatibility after decodeFlags then I'm wondering how to tell whether the flag was passed in via args or if the flag came from defaultFlags. Would it work if oFile :: String changes to oFile :: Maybe String, the default is Nothing, the Maybe is used to tell whether the flag was passed in or not, and later on in checkLinkFlags the oFile could be updated to the default Just "a.out" if the user hadn't already passed it in via args? Not sure if this is the way to go because then defaultFlags acts like an initialFlags and the default value gets assigned later on.

[brooksbp]

@quark17 ping!

[quark17]

Hm. You're right, that there's currently no way to tell if the user provided -o or not. Using a Maybe type is a reasonable why to fix it for this one flag. What's less clear to me is whether there is a way to address this more generally, since there are many flags that are likely unused if not linking (and vice versa).

It's worth noting that, for path flags, we use a token (defaultPathToken) to indicate the default value, which the user can either add to or replace. The default path can vary based on the flags given -- well, proprietary BSC had features (like BlueNoC/SceMi) that, when turned on via a flag, would add additional directories to the default path -- so BSC can't know the default path while decoding the flags one by one. It's when all the individual flags have been decoded that a cleanup function is applied (adjustFinalFlags) that can construct the default path and swap it in for any remaining tokens. This isn't too different from what you're proposing: the flag starts with Nothing and then gets replaced with Just "a.out" by a later function. The function adjustFinalFlags currently happens before the determination of Compile vs Link, so you couldn't put the replacement there -- unless we move adjustFinalFlags to be later (which might make sense, since some of the paths might only apply to linking, for example)? Putting the replacement in checkLinkFlags is probably reasonable?

Another option is to not replace it at all, leave it as Nothing. And then do the replacement when the value is needed.

Another option is to not use Maybe but to separately store a Bool for whether -o was provided on the command line. In some sense, that is information that is only needed at decode-time and is not needed by the rest of the compiler; so maybe it makes sense to have a separate data structure of state for flag decoding that is separate from the final config values that are passed to the rest of the compiler -- so, like Flags and FlagDecodeState. And, heck, maybe the return value from the flag decode stage, instead of being a single Flags structure, could be one of LinkFlags and CompileFlags etc, and these only have fields for the flags that are used by those modes.

Probably we should start by auditing all the flags and when they are used (and under what circumstances we'd want to warn/error). But if you just want to fix things for this one flags (and maybe it can be argued that this flag is one that people will trip over more?), then using a Maybe in the Flags field is probably OK for now.

[quark17]

It's also worth noting that source programs can attach flags to modules and functions, with an attribute (options), to be used when separately synthesizing that module/function. These are decoded using updateFlags, which calls the same decodeFlags function, to decode the individual flags. Only a few flags are relevant for elaboration; the rest (nearly all flags) will be accepted, but unused. Some flags might even be used, but are not what we want to allow -- for example, you might be able to change file paths, or even change the chosen backend!

If one is thinking generally about command-line decode issues, it might be worth starting by thinking about the attribute situation -- solutions for other issues might just fall out of any solution for this.