Re: [PATCH 58/79] OvmfPkg/Qemu: Fix various typos

["Laszlo Ersek" <lersek@redhat.com>]

On 12/09/19 14:00, Philippe Mathieu-Daudé wrote:

> I forgot, to have VbeShim.sh succeed, I also have this local change:
>
> -- >8 --
> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.sh
> b/OvmfPkg/QemuVideoDxe/VbeShim.sh
> index aea28be35f5c..92b345befe1f 100755
> --- a/OvmfPkg/QemuVideoDxe/VbeShim.sh
> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.sh
> @@ -28,7 +28,7 @@ trap exit_handler EXIT
>  #
>  # Assemble the source file.
>  #
> -nasm -o "$STEM".bin -- "$STEM".asm
> +nasm -fbin "$STEM".asm -o "$STEM".bin
>
>  #
>  # Disassemble it, in order to get a binary dump associated with the
> source.
> ---
>
> Because I had this error:
>
> $ sh -x OvmfPkg/QemuVideoDxe/VbeShim.sh
> [...]
> ++ dirname -- OvmfPkg/QemuVideoDxe/VbeShim.sh
> ++ basename -- OvmfPkg/QemuVideoDxe/VbeShim.sh .sh
> + STEM=OvmfPkg/QemuVideoDxe/VbeShim
> + trap exit_handler EXIT
> + nasm -o OvmfPkg/QemuVideoDxe/VbeShim.bin --
> OvmfPkg/QemuVideoDxe/VbeShim.asm
> nasm: error: more than one input file specified
> nasm: error: more than one input file specified
> nasm: error: more than one input file specified
> type `nasm -h' for help
> + exit_handler
>
> Which is rather confuse, because the help is still recommending the
> current form:
>
> $ nasm -h
> usage: nasm [-@ response file] [-o outfile] [-f format] [-l listfile]
>             [options...] [--] filename
>     or nasm -v (or --v) for version info
>
>     -t          assemble in SciTech TASM compatible mode
>     -E (or -e)  preprocess only (writes output to stdout by default)
>     -a          don't preprocess (assemble only)
>     -M          generate Makefile dependencies on stdout
>     -MG         d:o, missing files assumed generated
>     -MF <file>  set Makefile dependency file
>     -MD <file>  assemble and generate dependencies
>     -MT <file>  dependency target name
>     -MQ <file>  dependency target name (quoted)
>     -MP         emit phony target
>
>     -Z<file>    redirect error messages to file
>     -s          redirect error messages to stdout
>
>     -g          generate debugging information
>
>     -F format   select a debugging format
>
>     -gformat    same as -g -F format
>
>     -o outfile  write output to an outfile
>
>     -f format   select an output format
>
>     -l listfile write listing to a listfile
>
>     -I<path>    adds a pathname to the include file path
>     -O<digit>   optimize branch offsets
>                 -O0: No optimization
>                 -O1: Minimal optimization
>                 -Ox: Multipass optimization (default)
>
>     -P<file>    pre-includes a file
>     -D<macro>[=<value>] pre-defines a macro
>     -U<macro>   undefines a macro
>     -X<format>  specifies error reporting format (gnu or vc)
>     -w+foo      enables warning foo (equiv. -Wfoo)
>     -w-foo      disable warning foo (equiv. -Wno-foo)
>
>     -w[+-]error[=foo] can be used to promote warnings to errors
>     -h           show invocation summary and exit
>
> --prefix,--postfix
>                 these options prepend or append the given string
>                 to all extern and global variables
>

Yeah I think this is a NASM regression. In my version of NASM
(nasm-2.10.07-7.el7.x86_64, on RHEL7), the manual says, near "-f":

> To see a list of valid output formats, use the -hf option.

And when I invoke NASM like that, I get:

> valid output formats for -f are (`*' denotes default):
>   * bin       flat-form binary files (e.g. DOS .COM, .SYS)

So NASM either removed any default for "-f", or changed the default. Let
me check...

At upstream commit e91f5cc1322e ("preproc: fix %undef of macro aliases,
and add %ifdefalias", 2019-10-23), the source file "output/outform.h"
still contains:

> #ifndef OF_DEFAULT
> #define OF_DEFAULT of_bin
> #endif

This can be overridden when building NASM:

>  * Default config = -DOF_ALL -DOF_DEFAULT=of_bin

However, if I check Fedora dist-git at commit 0150c8afb52f ("Rebuilt for
Python 3.8", 2019-08-19) (the HEAD of the f30 branch), then I do *not*
see any OF_DEFAULT mentions.

...

NASM in Fedora 30 seems to be based on 2.14.02. Upstream tag "nasm-2.14"
contains commit 987dc9c9dbe3 ("Make any execution limit configurable,
add eval limit", 2018-06-12). This commit seems to have changed how the
"--" options-terminator is processed:

> @@ -1068,25 +1062,61 @@ static bool process_arg(char *p, char *q, int pass)
>          case '-':
>              {
>                  const struct textargs *tx;
> +                size_t olen, plen;
>
> -                if (p[2] == 0) {        /* -- => stop processing options */
> +                p += 2;
> +
> +                if (!*p) {        /* -- => stop processing options */
>                      stopoptions = true;
>                      break;
>                  }

I don't know if this is actually the problem, it just looks related. Can
you report a bug in the upstream tracker? <https://bugzilla.nasm.us/>

If NASM upstream confirms the bug, then I'd like to request that you
please submit this patch for "OvmfPkg/QemuVideoDxe/VbeShim.sh",
including a comment to the upstream NASM bug report.


To be clear, the original -- now apparently broken -- NASM command line
follows the "Utility Syntax Guidelines" from POSIX:

  https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02

See in particular Guideline 9 ("All options should precede operands on
the command line") and Guideline 10 ("The first '--' argument that is
not an option-argument should be accepted as a delimiter indicating the
end of options"). Namely:

* '-o "$STEM".bin' is an option, taking an option-argument. Therefore it
  should go first.

* The argument '--' is the end-of-options delimiter, which is generally
  recommended every time an operand is substituted. (For example, if we
  want to delete "$HELLO", we don't just write 'rm -rf "$HELLO"', but
  "rm -rf -- "$HELLO".)

* The argument '"$STEM".asm' is an operand.


The command line that your patch adds:

- breaks Guideline 9, because it intermixes options and operands;

- breaks Guideline 6, because it contracts "-f" with "bin" into a single
  argument, even though "-f" takes a mandatory -- i.e. not optional --
  option-argument (such as "bin"). Per guideline 6, if an option
  requires a mandatory option-argument, then the option itself, and its
  argument, should be separate arguments.

I don't meant that your patch is wrong -- I mean that NASM could be
wrong to require us to use this syntax. Hence my request to file a bug
report for upstream NASM first.

Thank you!
Laszlo