Wandering Thoughts archives

Official source release builds should not abort on (compilation) warnings

I will put my conclusion up front:

Official source releases should not build software with -Werror or the equivalent.

(By official source releases I mean things like 'we are releasing version 1.x.y of our package today'.)

Perhaps you disagree. Then the rest of this entry is for you.

As I write this, the current release version of Rust is 1.10.0. This version (and probably all previous ones) won't build on the recently released Fedora 24 because of a C compiler issue. This isn't because of bug in the Fedora 24 version of gcc, and it's not due to the Fedora 24 gcc uncovering a previously unrecognized bug in Rust. Instead it's because of, well, this:

src/rt/miniz.c: In function ‘tinfl_decompress’:
src/rt/miniz.c:578:9: error: this ‘for’ clause does not guard... [-Werror=misleading-indentation]
         for ( i = 0; i <= 143; ++i) *p++ = 8; for ( ; i <= 255; ++i) *p++ = 9; for ( ; i <= 279; ++i) *p++ = 7; for ( ; i <= 287; ++i) *p++ = 8;
         ^~~
src/rt/miniz.c:578:47: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘for’
         for ( i = 0; i <= 143; ++i) *p++ = 8; for ( ; i <= 255; ++i) *p++ = 9; for ( ; i <= 279; ++i) *p++ = 7; for ( ; i <= 287; ++i) *p++ = 8;
                                               ^~~
[...]
cc1: all warnings being treated as errors

Rust has opted to compile much or all of the C in its source tree with the gcc options -Wall -Werror, which mean 'emit more or less all warnings that you can, and if you emit any warnings consider this an error and stop'. Fedora 24 is one of the first Linux distributions to ship with gcc 6, and gcc 6 has added some more warnings, which now pick up more nits in the C code, and now Rust doesn't build.

It would be one thing if this was pointing out a previously undiscovered error. But it's not. The code in the Rust 1.10.0 release is merely not ideal, and this example neatly illustrates the problem with making 'not ideal' a fatal error in official source releases. Put simply, the compiler's definition of 'not ideal' changes over time.

When you set your official releases to build with -Wall -Werror or the equivalent, you're putting yourself on quicksand. It's basically guaranteed that future compiler versions will have different opinions on what to warn about, which means that official releases of your software are going to stop building at some point in the future for no good reason. Having your software stop building for no good reason is not helping people.

(I say 'for no good reason' because if it actually built, the release would be no more broken than it was before the new nits were reported. The nits were always there, after all, even on all the systems that didn't report them.)

I think it's fine to use -Werror in development if you want to. But the moment you make a release, my very strong sysadmin opinion is that the job of that release is to build correctly in as many environments as possible, including future environments. An attempt to build a release should fail on a system only if the end result would be incorrect in a new way.

(If a release is incorrect on both system X and system Y but this is only uncovered on system Y, that should not be a fatal error. It's sufficient for release builds to be bug for bug equivalent to each other. This is probably too tricky to do in most cases, although maybe you should provide a 'I don't care, just force it to build' configuration option that suppresses as many compiler errors as possible.)

Another way to put this is that -Wall -Werror is a service for the developers; it surfaces nits and forces developers to fix them. However, releases are not made for developers, they're made for outside people. Forcing developer issues on outside people is both futile (since outside people are not developers) and annoying. Thus actual releases should have things like compiler options reoriented to serve the needs of outside people instead of developers.

Your C compiler's optimizer can make your bad programs compile

Every so often I learn something by having something brought to my awareness. Today's version is that one surprising side effect of optimization in C compilers can be to make your program compile. The story starts with John Regehr's tweets:

@johnregehr: tonight I am annoyed by: a program that both gcc and clang compile at -O2 and neither compiles at -O0

@johnregehr: easier to arrange than you might think

void bar(void);
static int x;
int main(void) { if (x) bar(); }

(With the Fedora 23 versions of gcc 5.3.1 and clang 3.7.0, this will fail to compile at -O0 but compile at -O1 or higher.)

I had to think about this for a bit before the penny dropped and I realized where the problem was and why it happens this way. John Regehr means that this is the whole program (there are no other files), so bar() is undefined. However, x is always 0; it's initialized to 0 by the rules of C, it's not visible outside this file since it's static, and there's nothing here that creates a path to change it. With optimization, the compiler can thus turn main() into:

int main(void) { if (0) bar(); }

This makes the call to bar() unreachable, so the compiler removes it. This leaves the program with no references to bar(), so it can be linked even though bar() is not defined anywhere. Without optimization, an attempt to call bar() remains in the compiled code (even though it will never be reached in execution) and then linking fails with an error about 'undefined reference to `bar''.

(The optimized code that both gcc and clang generate boil down to 'int main(void) { return 0; }', as you'd expect. You can explore the actual code through the very handy GCC/clang compiler explorer.)

As reported on Twitter by Jed Davis, this can apparently happen accidentally in real code (in this case Firefox, with a C++ variant).

Some notes on adding exposed statistics to a (Go) program

As a slow little project for the past while, I have been adding some accessible statistics to my sinkhole SMTP server, using Go's expvar package. This has resulted in me learning lessons both about expvar in specific and the process of adding statistics in general.

My big learning experience is going to sound fairly obvious and trite: I only really figured out what statistics I wanted to expose through experimentation. I started out with the idea that counting some obvious things would be interesting (and to a certain extent they were), but I created many of the lot of stats by a process of looking at the current set and realizing that there was information I wanted to know or questions that I wanted answered that were not covered by existing things I was exposing. Sometimes trying to use the initial version of statistic showed me that it was too broad or needed some additional information in order to be useful.

The corollary to this is that what statistics you'll want depends in large part on what questions are interesting and informative for you, which depends on how you're using the program. A lot of my stats are focused on anti-spam related issues, because that's how I'm using my sinkhole SMTP server. Someone using it to collect email from a collection of nodes and tests might well want a significantly different set of statistics. This does make adding stats to a theoretically general program a somewhat tricky thing; I have no good answers to this currently.

(I have not tried to be particularly general in my current set of stats. Since this has been an experiment to play around with the idea, I've focused on making them interesting to me.)

Just exporting statistics from a program is less general than pushing events and metrics into a full time series based metrics system, but Go's expvar package and a few other tools like jq makes it much easier to do the former (for a start, I don't need a metrics system). Exporting statistics is also not as comprehensive as having an event log or the like. Since I do sort of have an event log, I've chosen to view my expvar stats as being an on-demand summary of it, one that I can look at without having to actively parse the log to count things up.

And on another obvious note, putting counters and so on in a hierarchical namespace is quite helpful for keeping things comprehensible and organized. To some extent a good hierarchy can substitute for not being able to come up with great names for individual statistics. And sometimes you have data with unpredictable names that has to be confined to a namespace.

(For instance, I track DNS blocklist hit counts. The names of DNS blocklists are essentially arbitrary, so I put the whole set of stats into a dnsbl_hits namespace. And because the expvar package automatically publishes some general Go stats on things like your program's memory usage, I put all of my stats under a top-level name so it's easy to pick them out.)

My approach for inspecting Go error values

Dave Cheney sort of recently wrote Don't just check errors, handle them gracefully, where he strongly suggested that basically you should never check the actual values of errors. This is generally a great idea, but sometimes you don't have a choice. For example, for a long time it was the case that the only way to tell if your DNS lookup had hit a temporary DNS error (such as 'no authoritative servers for this domain are responding') or a permanent one ('this name doesn't exist') was to examine the specific error that you received. While net.DNSError had a .Temporary() function, it didn't return true in enough cases; you had to go digging deeper to know.

(This was Go issue 8434 and has since been fixed, although it took a while.)

When I had to work around this issue (in code that I suppose I should now remove), I was at least smart enough to try the official way first:

 var serverrstr = "server misbehaving"

 func isTemporary(err error) bool {
    if e, ok := err.(*net.DNSError); ok {
       if e.Temporary() || e.Err == serverrstr {
          return true
       }
    }
    return false
 }

Checking the official way first made it so that once this issue was resolved, my code would immediately start relying on the official way. Checking the error string only for net.DNSError errors made sure that I wouldn't get false positives from other error types, which seemed like a good idea at the time.

When I wrote this code I felt reasonably smart about it; I thought I'd done about as well as I could. Then Dave Cheney's article showed me that I wasn't quite doing this right; as he says in one section ('Assert errors for behaviour, not type'), I should have really checked for .Temporary() through an interface instead of just directly checking the error as a net.DNSError. After all, maybe someday net.LookupMX() and company will return an additional type of error in some circumstances that has a .Temporary() method; if that would happen, my code here wouldn't work right.

(I even put some comments in musing about the idea, but then rejected it on the grounds that the current net package code didn't do that so there didn't seem to be any point. In retrospect that was the wrong position to take, because I wasn't thinking about potential future developments in the net package.)

I'm conflicted over whether to cast to specific error types if you have to check the actual error value in some way (as I do here). I think it comes down to which way is safer for the code to fail. If you check the value through error.Error(), future changes in the code you're calling may cause you to match on things that aren't the specific error type you're expecting. Sometimes this will be the right answer and sometimes it will be the wrong one, so you have to weigh the harm of a false positive against the harm of a false negative.

Go does not have atomic variables, only atomic access to variables

Suppose, hypothetically, that you have a structure full of expvar variables. You would like to expose all of them in one operation with expvar.Func, using some code that goes roughly like this:

var events struct {
   Var1, Var2, Var3 expvar.Int
}

func ReportStats() interface{} {
   return events
}

Ignoring for the moment how this won't work, let's ask a more fundamental question: is this a safe, race-free operation?

On first blush it looks like it should be, since the expvar types are all safe for concurrent access through their methods. However, this is actually not the case, due to an important thing about Go:

Go does not have atomic variables, only atomic access to variables.

Some languages support special atomic variable types. These variable types are defined so that absolutely all (safe) language access to the variables that you can perform is atomic, even mundane accesses like 'acopy = avar' or 'return avar'. In such a language, ReportStats() would be safe.

Go is not such a language. Go has no atomic variable types; instead, all it has is atomic access to ordinary non-atomic variables (through the sync/atomic package). This means that language level operations like 'acopy = avar' or 'return avar' are not atomic and are not protected against various sorts of data races that create inconsistencies or other dangers. The expvar types are no exception to this; their public methods are concurrency safe (which is achieved in various ways), but the actual underlying unexported fields inside them are not safe if you do things like make copies of them, as ReportStats() does when it says 'return events'.

In some cases you can get a warning about this, as go vet will complain about the related issue of making a copy of a sync lock (or anything containing one, such as a sync.RWMutex). Some types that are intended to be accessed only atomically will have an embedded lock, and so making a copy of them will cause go vet to complain about copying their embedded lock. However, not all 'intended to be atomic' types use embedded locks, so not all will be caught by this check; for example, expvar.String has an embedded lock (in a sync.RWMutex) and so will provoke go vet complaints when copied, but expvar.Int currently doesn't have an embedded lock and go vet will not warn you if you copy one and give yourself a potential data race.

(There may someday be a way to annotate types so that go vet knows to complain about making copies of them, but as far as I know there's no way to do that today. If such a way is added, presumably all expvar variable types would get that annotation.)

A quick trick: using Go structs to create namespaces

Suppose, not entirely hypothetically, that you have a bunch of expvar statistics variables in your program that you're registering yourself in order to create good names for them in the exposed JSON. Implemented normally, this probably leaves you with a bunch of global variables for various things that your program is tracking. Just like any other jumble of global variables, this is unaesthetic and it would be nice if we could do better.

Well, we can, due to Go's support for unnamed struct types. We can use this to basically create a namespace for a collection of variables:

var events struct {
   connections, messages  expvar.Int
   tlsconns, tlserrors    expvar.Int
}

In our code we can now refer to events.connections and so on, instead of having to have more awkward or more ambiguous names.

We aren't restricted to doing this at the global level, either. You can fence off any set of names inside such a namespacing struct. One example is counters embedded into another structure:

type ipMap struct {
   sync.Mutex
   ips   map[string]int
   stats struct {
       Size, Adds, Lookups, Dels  int
   }
}

For obvious reasons, this works best for variable types that don't need to be initialized; otherwise things get at least a least a little bit awkward with how you have to set up the initialization.

This is probably not to everyone's tastes and I don't know if it's considered good Go practice. My personal view is that I would rather fence off variable names with 'prefix.' than with say 'prefix_', even at the cost of introducing such an artificial unnamed struct, but other people probably differ here. It does feel a bit like a hack even to me, but maybe it's a legitimate one.

(For statistics counters specifically, this can also give you a convenient way of exposing them all.)

Out of curiosity I did a quick scan of the current development Go compiler and standard library and turned up a few things here and there that might be this pattern. There are variations and it's not all that common, so the most I'm going to say based on looking is that this doesn't seem like a completely outrageous and unsupported idea.

You can't use expvar.Func to expose a bunch of expvar types

Suppose, hypothetically, that you have a collection of statistics that are each one of the expvar package's types. You want to put them in a namespace (so they are all 'mything.var1', 'mything.var2' and so on), but you'd like to avoid the tedious clutter of registering each expvar variable by hand with .Set(). So you have a clever idea.

First, you will embed all of the variables in a structure:

var somestats struct {
   var1, var2, var3   expvar.Int
   var4, var5, var6   expvar.Int
}

Then you will write a Stats() function that simply hands back the structure and then register it through expvar.Func():

func ReportStats() interface{} {
   return somestats
}

Unfortunately, this will not work (at least today). As I mentioned in my first set of notes on the expvar package, expvar.Func turns what your function returns into JSON by using json.Marshal, and this only returns exported fields. None of the expvar variable types have any exported fields, and so as a result expvar.Func() will convert them all to empty JSON (or possibly malfunction). You just can't get there from here.

This is kind of a bug (at a minimum, expvar.Func should document this restriction), but it's unlikely to change (apart from the documentation being updated). Beyond it not working today, there's no way to have a simple ReportStats function like this that work safely, and since you can't do this there's little to no point in making expvar variable types JSON-serializable through json.Marshal.

(To make this work, each expvar type would implement a MarshalJSON() method that did the appropriate thing. In fact, since expvar.String is really MarshalJSON() in disguise, you could just make one call the other.)

Sidebar: Why clear and complete documentation matters

Here is a question: is it deliberate that the 'thing to JSON' function for expvar.Var is not called MarshalJSON, or is it a historical accident? You can certainly argue that because my pattern above is fundamentally wrong, it's a feature that it doesn't work at all. Thus the choice of not using MarshalJSON in the expvar.Var interface could be entirely deliberate and informed, since it makes a broken thing (and all its variants) simply not work. Or this could be ultimately a mistake on the order of using String() in the expvar.Var interface, and so something that would be corrected in a hypothetical Go 2 (which is allowed to change APIs).

Without better documentation, people who come along to Go later just don't know. If you want to preserve the intent of the original designers of the language and the standard library, it really helps to be clear on what that intent is and perhaps the logic behind it. Otherwise it's hard to tell deliberate decisions from accidents.

Some notes on Go's expvar package

I recently decided to add accessible statistics to my sinkhole SMTP server by using the expvar package. In the process of experimenting with this, I've wound up with a collection of notes that I want to write down while I remember them.

First up is my discovery about expvar.Var, namely that it wants you to return properly quoted JSON instead of just a string. You can read the entry for more details.

The expvar package exposes a lot of 'New<whatever>' functions to give you new variables of various types. The important thing to bear in mind about these is that they bundle together both the actual variable and its expvar name. This means that they will always be separate top level JSON objects; you can't put them inside an application-wide or package-wide JSON dictionary or the like.

(This is the difference between having a 'memstats' JSON dictionary with elements like 'Alloc' and 'BySize', and having separate 'memstats-Alloc', 'memstats-BySize', and so on JSON things.)

In my view, you generally want to namespace all your variables to make them easier to deal with. To do namespacing, you don't use 'New<whatever>', except for one top level map (that will become a JSON dictionary); instead you create instances of whatever you need and then manually embed them in your namespace map. Here, have an example:

var counter1, counter2 expvar.Int
var status expvar.String

m := expvar.NewMap("myapp")
m.Set("counter1", &counter1)
m.Set("counter2", &counter2)
m.Set("status", &status)

Most expvar types can be used this way without explicit initialization. The exception is expvar.Map, which requires you to call its .Init() before you do anything else with it:

var submap expvar.Map
submap.Init()
submap.Set("somevar", &somevar)
submap.Set("somevar2", &somevar2)
m.Set("subthing", &submap)

The other way of setting up variables or in fact entire hierarchies of information is to bulk-export them through a function by using expvar.Func. Often such a function will be handing over an existing structure that you maintain. For example:

type iMap struct {
   sync.Mutex
   ips   map[string]int
   stats struct {
      Size, Adds, Lookups, Dels int
   }
}

func (i *iMap) Stats() interface{} {
   i.Lock()
   defer i.Unlock()
   i.stats.Size = len(i.ips)
   return i.stats
}

var tlserrs iMap

Then, to create this as a variable in your application's namespace map:

m.Set("tlserrors", expvar.Func(tlserrs.Stats))

One important note here is that the conversion to JSON is done using json.Marshal and is subject to its rules about field visibility. I believe that this means all fields need to be exported, as they are in this example (or at least all fields that you want to be visible as expvar variables). This is not a requirement for things that you register explicitly or set up through expvar.New<whatever>; as in my examples above, they can be non-exported variables and usually are.

(This is how the standard expvar cmdline and meminfo variables are implemented.)

Sidebar: Locking and statistics

My first version of the code I wrote here had a subtle bug. The original version before I added statistics sensibly used sync.RWMutex and took only a read lock during lookups; a write lock was only necessary when adding or deleting entries (a rarer activity). But when I added statistics variables using non-atomic operations, lookups were suddenly doing writes and so now actually needed to be protected with a write lock. Which meant everything needed a write lock, which is why the code here uses a plain sync.Mutex instead.

For subtle reasons beyond the scope of this sidebar, I don't think you can get around all concurrency races here by using expvar types (which are all normally safe against multiple updates); as far as I can see, the .Stats() function itself has races.

(Note that in general, global statistics are always a contention point if you have high concurrency.)

A surprise to watch out for with Go's expvar package (in expvar.Var)

The standard expvar package is a handy thing for easily exposing counters, values, and so on in a way that can be queried from outside your running program. As you might expect, it ultimately works through an interface type, expvar.Var. This interface is very simple:

type Var interface {
    String() string
}

If you see this definition, your eyes may light up with familiarity (as mine did), because this is exactly the extremely standard fmt.Stringer interface, where everything that has a .String() method can be handled by a lot of things. So of course you might well write code like this:

m := expvar.NewMap("myapp")
// the time.Time type has a String()
// method, so this will totally work.
m.Set("startTime", time.Now())

If you do this, everything will work right up to the point where programs that parse the JSON returned by the /debug/vars endpoint start failing with weird errors. If you look at the raw JSON, what you will see is something like this:

[...], "startTime": 2016-04-01 17:49:09.385829528 -0400 EDT, "anothervar": "something", [...]

In case you don't see the problem (as I didn't for some time), the string value for "startTime" doesn't have quotes around it, which makes it very invalid JSON. Go's current JSON parser starts trying to interpret the starting '2016-' bit as a number, then runs into the '-' and complains about it.

What is happening is that the expvar.Var String() interface method is misnamed; it should really be called something like JSON(). What the Var.String() method is actually required to do is produce the JSON representation of the Var as a string; for strings, this requires them to be quoted. A normal Stringer .String() method doesn't do this quoting, of course, because it would get in the way. The two interpretations of .String() are not really compatible, but there is no way to tell them apart and Go's implicitly satisfied interfaces will let you substitute one for the other (as I did when I tried to use time.Time as a Var).

So the takeaway here is that just because something has a .String() doesn't mean you can use it as an expvar.Var; in fact, you probably can't. Anything that's designed to be used as an expvar.Var will specifically say so in its documentation (or at least it should). Anything that has a .String() but doesn't mention expvar should be assumed to be satisfying the far more common fmt.Stringer interface instead.

(I don't have any clever solution for this. And I think the Go 1 API compatibility guarantee will keep this from changing, as expvar.Var was in its current form in the initial Go 1 release.)

When you want non-mutating methods in Go

In Should methods be declared on T or *T, Dave Cheney included a footnote:

5. If the method does not mutate its receiver, does it need to be a method?

(One alternative Dave Cheney had in mind was a pure function that's passed the necessary fields directly, cf. I also suspect that Dave Cheney put this in more to make readers think about the issue than as an outright directive.)

I'll bite on that somewhat rhetorical question, because I see several possible reasons to want a method that merely reads things from the receiver without modifying it.

The biggest one is interfaces. Interfaces can only contain methods, so if you want an interface to give you some form of polymorphism over fields you need either getter methods or a method that does the actual field based computation. We can see this all over the Go standard library; for instance net.Conn contains some functions that are more or less getters, namely LocalAddr() and RemoteAddr().

Even without interfaces in the picture, encapsulating fields inside method functions preserves your future freedom of action. Maybe the computation involving fields is simple today and easily done directly by outside people, or maybe today the getter functions are trivial. But tomorrow the calculations could be complex or the field values generated through non-trivial calculations; in both cases, you'd want to change the API away from direct field access. Obviously this is less compelling when it involves fields that aren't exported outside your package, because then they aren't part of any public API. So there I'm with Dave Cheney, because this is at least something to think about.

Finally, you may reasonably feel that having read-only method functions simply makes the (internal) API simpler and more regular. There's definitely a consistency value in always doing x.thing() instead of sometimes doing x.writething() and other times doing readthing(x.field1, x.field2). And if you're tempted to write just readthing(x) I would definitely make it a method, because x.readthing() and readthing(x) are almost directly equivalent.