Dealing with autogenerated files - poll

Parameters are greppable by their parameter name. Drop the leading OSSL_ and you'll find it. Moreover, you never need to reference the name under the new world order. It appears in the table of parameters and that's it. You use the matching field name exclusively.

If we really want the OSSL_ prefix included too it, it's possible. I took them out when I first introduced the paramnames.pm script a long time ago. I do wonder why it becomes necessary now when the param names are pretty much not used anymore?

Having worked with the new scheme more than anyone, I find that I almost never look at the generated file. There is no need to if the generated code does what it's meant to. The only difficulty is that the line numbers in the generated file don't match the ones in the source -- this is what we need to address so that we never look at the generated code. Someone needs to figure out how to insert appropriate #line directives into the generated file for the C compiler.

Just yesterday I had to answer to my colleagues when they asked "How does openssl deal with this parameter?". A week ago a different colleague has the same question. This is a major downside of your approach, unfortunately.

I will admit that it's slightly different to how you'd previously have found it but it doesn't require looking at the generated file. Let's look for OSSL_KDF_PARAM_ITER in PBKDF2. Bring up pbkdf2.c.in and search for KDF_PARAM_ITER. It's on line 291 or thereabouts:

['KDF_PARAM_ITER', 'iter', 'uint64'],

Which is from this block that generates the structure, decoder and parameter list:

{- produce_param_decoder('pbkdf2_set_ctx_params',
(['KDF_PARAM_PROPERTIES',  'propq',    'utf8_string'],
 ['ALG_PARAM_ENGINE',      'engine',   'utf8_string', 'hidden'],
 ['KDF_PARAM_DIGEST',      'digest',   'utf8_string'],
 ['KDF_PARAM_PASSWORD',    'pw',       'octet_string'],
 ['KDF_PARAM_SALT',        'salt',     'octet_string'],
 ['KDF_PARAM_ITER',        'iter',     'uint64'],
 ['KDF_PARAM_PKCS5',       'pkcs5',    'int'],
)); -}

Now search for the field name: iter. Its code starts from line 342:

    if (p.iter != NULL) {
        if (!OSSL_PARAM_get_uint64(p.iter, &iter))
            return 0;
        if (!lower_bound_check_passed(ctx, INT_MAX, iter, SIZE_MAX,
                                      ctx->lower_bound_checks))
            return 0;
        ctx->iter = iter;
    }

The pointer to the parameter has been set by the decode call and it's NULL if it wasn't present.

We'll need a CI check if the generated files are checked in. Consider what happens if the .c is accidentally modified. It can no longer be correctly generated from the .c.in file. Worst case is if both the .c and the .c.in are changed and committed -- here something will be lost on regeneration.

I'm really disturbed by (2) and (3).

@Dmitry Belyavsky 3) could be solved by changing the generator to use the full param name in the produce_param_decoder() call.

And I believe that it might solve 2) partially as well because when you then search for the full param name you get the template file as a result which should hint you that the .c file is generated. Also 2) can be solved by making the generated files read-only or by using out-of-tree builds which is a good idea regardless.

3 would be better solved by using the full param name in the table of params and the paramgen script. In hindsight, I shouldn't have omitted the OSSL_ prefix.

There should be zero need to care about the .c files or even know that they exist. Once this is achieved, problem 2 simply goes away.

I still have no idea how looking at the generated .c files to see how a param is being processed helps, in any way, to locate where & how it is used. All it does is assign the param to a pointer in the structure, quite literally "r->name = p;"

The param name is included in the decoder as a comment in the appropriate location. 3 would be better solved by using the full param name in the table of params and the paramgen script. In hindsight, I shouldn't have omitted the OSSL_ prefix.

There should be zero need to care about the .c files or even know that they exist. Once this is achieved, problem 2 simply goes away.

I still have no idea how looking at the generated .c files to see how a param is being processed helps, in any way, to locate where & how it is used. All it does is assign the param to a pointer in the structure, quite literally "r->name = p;"

@Paul Dale thanks for resolving (3)

@Tomas Mraz Your ideas to resolve (2) seem reasonable but I think not many people use out-of-tree builds

Rather than out of tree builds (which I only use :), making the generated files read only should work.

Rather than out of tree builds (which I only use :), making the generated files read only should work.

My resolution for (3) ought to go to 3.6.

Rather than out of tree builds (which I only use :), making the generated files read only should work.

My resolution for (3) ought to go to 3.6 as well as master to avoid a pustular blister in the code base.

@Paul Dale, could making the generated files read-only be made conditional on if they're part of a release? It's pretty easy to detect a release by looking at 'RELEASE_DATE' from VERSION.dat, which also finds its way to $config{releast_date} in perl.

The reason I'm saying this is that an OpenSSL developer between releases will otherwise constantly have to ensure the generated files are writeable, 'cause any update to the git working directory may potentially lead to them being regenerated, and that would be such a pain.

The regeneration would change permissions and overwrite.

Making them read only is to stop you, the programmer, from editing them and later losing your changes.

@Dmitry Belyavsky with point (2) addressed, is there still a high likelihood of mistakeningly editing the .c files?

How do you address it?

@Paul Dale I understand your great intention but introducing huge amount of preprocessable files that potentially should be changed (and the changes are not easily reparable, comparing to header files) looks fishy to me

I think this poll is misguided (sorry Dmitri), the problem that needs solving won't be solved by having the generated files included in the tree -- that will help a little but will also introduce other problems. To my mind it's the learning curve and the mistakes inherent in it that are the real problem. We need documentation about how this all works with examples so that new comers get a more gradual introduction to what is automated and how it all hangs together. We will also need coders to read the documentation but that's a harder problem…

I think we do need to get the compiler error message line numbers correct. With this there really will be no need to ever inspect the generated code.

1) Would require some trickery to get the compiler to report the error in the .in file,.
2) could be addressed by making the generated file read only..
3) Comments could solve this.

You could possibly edit either the .in file and/or the generated file and then up with merge conflicts in both. If this has any chance of producing more files that behave like the generated .num file then that would be really annoying.

@Shane Lontis 
> 3) Comments could solve this.

Er... have you looked? We usually have a warning comment at the top of the files. For example, include/openssl/asn1.h:

```
/*
* WARNING: do not edit!
* Generated by Makefile from ../include/openssl/asn1.h.in
*
```

That does require you to notice the warnings (which may depend on how you traverse the file)
The '.in' files dont have these generated warnings though?

@Shane Lontis

Not sure why the '.in' files would need warnings against editing them?

What they have is the perl droplet that generates those warnings. Specifically, in include/openssl/asn1.h.in, it looks like this:

/*
 * {- join("\n * ", @autowarntext) -}
 *

Perhaps I misunderstand what "Comments could solve this" was meant to imply

Keeping the .c files in the tree also leads to the question of when the .c.in files are converted to .c ones. Currently, this happens are part of the make process. I.e. any changes to a .c.in file are immediately represented in the corresponding .c file. If this is maintained, changes to the .c file will be immediately lost. If this is not maintained, the burden when working on these files is significant -- one would have to remember to make update or similar after every edit.

With the .c files in the tree, the best option would be for make to error out if the .c file has been edited. This could be done with some trickery but it's not going to be straightforward.

I'm ambivalent, and find myself unable to vote with confidence.
I'm thinking "yes" for files that do not depend on build configuration, "no" for those who do. For files where I say "yes", a simple "make update" should update them.

But that doesn't really help the core issue, like at all.

During yesterday's Foundation TAC meeting, someone (nope, not me) came up with an easy enough idea for this, leading to a 10 minute PR: https://github.com/openssl/openssl/pull/28612

I suggested this very early on.

Let's see if it makes things better or not.