Wireshark · Wireshark-dev: Re: [Wireshark-dev] Troubles with ASN generated code

[Pascal Quantin <pascal.quantin@xxxxxxxxx>]

2017-05-19 19:44 GMT+02:00 Guy Harris <guy@xxxxxxxxxxxx>:

On May 19, 2017, at 9:38 AM, Jaap Keuter <jaap.keuter@xxxxxxxxx> wrote:

> So, this change https://code.wireshark.org/review/#/c/21708/ appears to make the
> buildbot happy, the GCC6 builds still fails with
> (3346fc9c83719bce26cca31fc4b5d9139ab56d76)
>
> In file included from ./asn1/q932/packet-q932-template.c:33:0:
> ./asn1/q932/packet-q932-exp.h:26:27: error:
> ‘q932_PresentedNumberUnscreened_vals’ defined but not used
> [-Werror=unused-const-variable=]
> ./asn1/q932/packet-q932-exp.h:18:27: error: ‘q932_PresentedNumberScreened_vals’
> defined but not used [-Werror=unused-const-variable=]
> ./asn1/q932/packet-q932-exp.h:10:27: error:
> ‘q932_PresentedAddressUnscreened_vals’ defined but not used
> [-Werror=unused-const-variable=]
> ./asn1/q932/packet-q932-exp.h:2:27: error: ‘q932_PresentedAddressScreened_vals’
> defined but not used [-Werror=unused-const-variable=]

GCC 6 is probably correct.

*Defining* functions and data (except perhaps for inline functions) in a header file is generally Not The Right Thing To Do, because

        1) if only one source file includes the header, and there's no reason for other files to include it, the very existence of the header is misleading, as it implies that the stuff in the header is stuff being exported from the file;

        2) if more than one source file includes the header, you have multiple copies of the data or function, rather than sharing a single copy, which is arguably wasteful;

        3) if more than one file includes the header, and not all of them use all of the data or functions, you have multiple copies of the data or function *and some of them are unused*, which is *definitely* wasteful.

The only reason why you *might* want to do this for data is that, if you have a plugin module, and it has a data structure that points to one of the data structures mentioned in the header, the run-time loader on some platform you support might not do the load-time evaluation of the address of the structure required to make this work.  For example, if libwireshark exports a value_string table, I'm not sure all the platforms we support would allow a plugin dissector to point to that value_string from one of its header fields.

Given that we're already exporting value_string tables from libwireshark, either

        1) it works on all platforms

or

        2) we're not doing the stuff that doesn't work

so I wouldn't be opposed to doing with asn2wrs-generated dissectors the same way we do it for hand-written dissectors, i.e.:

        have the header file declare but *not* define the value_strings, and declare it with WS_DLL_PUBLIC rather than either static or extern;

        have the source files define the value strings, and define them with WS_DLL_PUBLIC_DEF.

Unfortunately this does not seem to work (at least with MSVC2013). I get the following error when trying to include a value_string from a plugin:

 error C2099: initializer is not a constant

So it was unnoticed up to now and the comments in ws_symbols_export.h are not (any longer) correct.

 

Any value_string that asn2wrs is not *absolutely certain* will be used only by one source file should *not* be made static and should *not* be defined in a header file.  Any value string that asn2wrs *is* absolutely certain will be used only by one source file should be defined as static *in that source file*, *not* in a header file.

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request@wireshark.org?subject=unsubscribe