Re: [PATCH] Make grub_strtoul() "end" pointer have the right const params.

[Peter Jones]

On Tue, Feb 04, 2020 at 08:04:30PM -0500, Nicholas Vinson wrote:
> On 2/4/20 16:02, Peter Jones wrote:
> > grub_strtoul() and grub_strtoull() don't make the /pointer/ to "end" be
> > const like normal implementations do, and as a result, at many places in
> 
> grub_strtoul() and grub_strtoull() appear to be patterned after the C
> standard functions strtoul() and strtoull().
> 
> According to C18, the signatures for those functions are:
> 
>     unsigned long int strtoul(const char * restrict nptr,
>                               char ** restrict endptr, int base);
> 
>     unsigned long long int strtoull(const char * restrict nptr,
>                                     char ** restrict endptr, int base);

It's true that the standard does not mandate any const usage on the
endptr parameter, and it does have restrict.  I'm happy to add restrict,
though I'm going to ignore it for the rest of this email, because the
standard literally says it changes no semantics.  Having "const" on both the
object pointed to by **endptr and on the outer pointer is still
compatible with the standard, though - nothing that's allowed by the
standard isn't allowed with those qualifiers.  Semantically, the
behavior is described in a way that is compatible with making those
const as well: the value written to *endptr must point inside the object
pointed to by nptr, and neither function is allowed to modify that
object.

Adding the const requirement does give us benefits though:
- It means when someone is modifying grub_strtoul() or grub_strtoull(),
  as inevitably happens every once in a while, making an error which
  would violate those requirements will cause an error building that
  change, rather than making it in to the tree.  That is, "const char **"
  protects us from making an error that writes modifies the object nptr
  points at, and "char ** const" protects us from accidentally changing
  the local pointer instead of the caller's pointer.
- Likewise, if someone isn't thinking through what they're doing and
  takes the const out in order to "fix" some perceived problem, that
  should alert whomever is reviewing the code.  In both of these
  regards, having the const qualifiers helps us trust that our
  implementation is correct.
- It lets us pass in pointers that we *want* to be const from the
  caller's point of view.  It means that if a function calling strtoul()
  has a "const char *" argument, it can safely pass that as nptr as well
  as passing its address as endptr; without making the object const (i.e
  at least "const char ** endptr"), that'll get you a warning about
  discarding the type qualifier.  That applies equally to cases that
  want to wrap these functions with another function that has similar
  semantics; having const qualifiers allows that function to declare
  those things const or not.  Not having the qualifiers here means the
  calling function also can't, which is where we wind up having a lot of
  the ugly code.
- The same applies to cases where the object is (or should be) genuinely
  immutable - for instance, data that's in .rodata, in data provided
  by an option ROM, or firmware configuration table.  It is better to be
  able to keep the type qualifier throughout the whole stack.
- It allows the optimizer to make assumptions about how we're using it
  which may allow it to generate more efficient output without having to
  analyze the caller.

> Therefore, I recommend the GRUB maintainers keep the existing signatures for
> grub_strtoul() and grub_strtoull() as they more closely follow the
> signatures of the standard functions.

Obviously, I don't agree.

> I also skimmed through the rest of the changes and it seems to me most of
> the misuse of these calls could be solved simply by doing some variation of:
> 
>     const char *ptr = ...;
>     char *end;
> 
>     strtoul(ptr, &end, 10);
>     ptr = end;

While this is valid in many of the existing cases, it's not as good in
several ways.  In addition to the last three points I made above, this
also creates an unnecessary alias into the character object, which the
compiler then has to track.  In this case, since it is both a qualified
type of the same type of object and a character object, it doesn't
violate the aliasing rules ("-fstrict-aliasing -Wstrict-aliasing" isn't
allowed to complain), but it can cause the compiler to both take longer
and generate more, slower code than if that were:

  const char *ptr = ...;
  unsigned long val;

  val = strtoul(ptr, &ptr, 10);

That's true for both the caller and in the implementation of strtoul()
itself - though I don't think with current GCC it *will* cause the
strtoul() output to be much different, because it's pretty simple, but
for a more complex function like sscanf() calling it, the chances are a
bit rougher.  Hypothetically, it can also reduce the chances that it
winds up getting inlined if we were to use -flto.

> which is what I would personally recommend instead.  If the compiler
> issues an error or warning with the above, then that's a bug the
> compiler maintainers should fix.

That's true, but it puts limits on the caller's usage and the compiler's
output.

-- 
  Peter