open-watcom / open-watcom-v2

Open Watcom V2.0 - Source code repository, Wiki, Latest Binary build, Archived builds including all installers for download.
Other
991 stars 163 forks source link

as: Implement "-fr" for error report files #1299

Closed winspool closed 5 months ago

winspool commented 6 months ago

The "-fr" argument breaks all risc assembler with owcc (see bug #1265)

The "-fr" argument breaks dependency creation for some autotools projects with owcc (LTP: Linux Test Project as example)

owcc detects the argument "-fr", but that's incomplete and undocumented https://github.com/open-watcom/open-watcom-v2/blob/1400d891465a25d8e25cdee657aff51352149138/bld/wcl/c/owcc.c#L727

Supporting "-fr[=\<file>]" in the RISC assembler or in owcc is an area for future enhancements.

-- Regards ... Detlef

jmalak commented 6 months ago

It cannot be accepted, it brokes backward compatibility. I explained you several times that your wish and change must follow OW rules, you ignore it. Anyway it is not a bug, nowhere described such functionality. The problem is simple, RISC assemblers don't support -fr option that you cannot use it with these assemblers. If you want resolve this issue then add support for -fr option to RISC assemblers. Only this has sense and don't break any existing functionality. Don't try to change processing logic of options in owcc tools. Anyway owcc is driver only that something as "slave" has no sense.

winspool commented 6 months ago

As you suggested, i implemented a minimal "-fr" support in the RISC assemblers, to allow them to work with owcc (see bug #1265).

Only "-fr" is accepted to disable the error report file. Arguments to "-fr" are not allowed.

A documentation for wasaxp/wasmps/wasppc is not present yet. I put it on my TODO list

jmalak commented 6 months ago

Sorry, again you do things your way, not how OW works. Read documentation for -fr option for compilers and WASM then do things properly. If you will ignore notes then I will close such uselless PRs without comments.

winspool commented 6 months ago

OK. I wil dsend my full "-fr" implementation

jmalak commented 6 months ago

It cannot be full implemetation, but must be done properly. Again you change processing logic incorrectly. -fr option cannot suppress anything it can only enable thing.

winspool commented 5 months ago

I ask for your hints, before pushing:

0001-as-Implement-argument-fr-for-the-error-report-file.txt

I used strncpy() to avoit a buffer overflow. Is this and the rest of my implementation ok?

wasm accepts also "-fe". should i add that also?

0002-as-Add-fr-option-to-the-commandline-help.txt

I removed the broken path parameter for "-oe" and "-oc", but both help texts are in a different style compared to the other entries. My suggestion: "create ELF object files" (similar for COFF) (or do you like "use ELF object file format" more?)

winspool commented 5 months ago

It cannot be full implemetation,

I tested my implementation, but as before:

but must be done properly.

The attached patches should match your requirements.

Again you change processing logic incorrectly. -fr option cannot suppress anything it can only enable thing.

In one of the files is documented, that "-fr" without an argument disables the error file. That's what i remembered, when i did my prev. pull request, and i think, that might be the reason, why owcc adds the "-fr" argument without a name to all client programs (when owcc did not find a "-fr" on it's own commandline).

I will seach tomorrow, where i did read it. That text might be wrong or outdated, but i can't know that. My OpenWatcom history is too short.

winspool commented 5 months ago

Again you change processing logic incorrectly. -fr option cannot suppress anything it can only enable thing.

In one of the files is documented, that "-fr" without an argument disables the error file.

I found it in the cguide.pdf on page 35: If no part of the name is specified, then no error file is produced (i.e., -fr disables production of an error file).

Please don't blame me, that I remembered that part of the documentation and that I used this logic to implement "-fr" without arguments in the RISC assemblers.

jmalak commented 5 months ago

It looks like you need flaw again as usual. You didn't read documentation correctly and write some nonsense. I don't want study your code for all incorrect things, I suppose if you want to change something that you do

  1. study documentation
  2. study appropriate code
  3. understand OW design rules

The logic is simple for -fr

  1. error file is enabled by default and use "*" template for file name and change extension to .err
  2. disable error file if no argument
  3. if argument exist then it is taken as template for error file name (from source file name)

The conclusion

  1. the WASM has bug in -fr processing by design, must be fixed or re-implemented
  2. the RISC assemblers need to implement functionality described above or ignore (skip) -fr option at all

The title of this PR is nonsens, because the core issue has nothing to do with owcc and what you write. Simple it is changes and fixes for -fr option processing in assemblers to follow compilers -fr processing to works properly with OW driver tools (wcl..., owcc).

Again, don't mix unrelated change to single PR. I don't accept PR with such mixture of changes.

winspool commented 5 months ago

I don't want study your code for all incorrect things,

Please do, even my code might be wrong or when you think, that my idea is nonsense. My intention is still to help to improve OpenWatcom, but the OpenWatcom codebase is huge and complicated. (And it's very different from other projects) I already learned some things, but a lot more is unknown to me.

Whith your OpenWatcom experience, my trials might look stupid, but I am sure, my code will be better over time, when i understand the OpenWatcom codebase and design decisions better.

The logic is simple for -fr

  1. error file is enabled by default and use "*" template for file name and change extension to .err

That's what my patch implements

  1. disable error file if no argument

That's what my patch implements

  1. if argument exist then it is taken as template for error file name (from source file name)

That's what my patch implements

Examples:

cd bld/as
$WATCOM/binl/wasppc  ppc/test/hello.asm
-> works without a failure

$WATCOM/binl/wasppc  mps/test/hello.asm
-> hello.err created

$WATCOM/binl/wasppc -fr  mps/test/hello.asm
-> error file disabled

$WATCOM/binl/wasppc -fr=.txt  mps/test/hello.asm
-> hello.txt created

$WATCOM/binl/wasppc  -fr=ppc/  mps/test/hello.asm
-> ppc/hello.err created

$WATCOM/binl/wasppc  -fr=$OWROOT/  mps/test/hello.asm
-> hello.err created in $OWROOT

$WATCOM/binl/wasppc  -fr=ppc/  mps/test/hello.asm
-> ppc/hello.err created

$WATCOM/binl/wasppc  -fr=/root/  mps/test/hello.asm
-> nothing created. No "access denied" failure message on the console

$WATCOM/binl/wasppc  -fr=does_not_exist/  mps/test/hello.asm
-> nothing created. No "directory not found" failure message on the console

$WATCOM/binl/wasppc  -fr=ppc/.log  mps/test/hello.asm
-> ppc/hello.log created

$WATCOM/binl/wasppc  -fr=report.log  mps/test/hello.asm
-> report.log created

as previously created always an error file, and deleted it after assembling, when no warning/error message was written.

After my patch, this is skipped, when "-fr" was given. Is this OK, or should the RISC assemblers always delete an errorfile after assembling?

The conclusion

  1. the RISC assemblers need to implement functionality described above or ignore (skip) -fr option at all

"-fr" is implemented as described above.

The title of this PR is nonsens, because the core issue has nothing to do with owcc and what you write.

I already changed the title multiple times. The mention of owcc was for my first try, when i removed the code in owcc (which was adding "-fr" to the called programs by default).

Simple it is changes and fixes for -fr option processing in assemblers to follow compilers -fr processing to works properly with OW driver tools (wcl..., owcc).

"-fr" is supported, when called the RISC assemblers directly.

owcc only accepts "-fr" and ignores optional parameter of "-fr" (handling of "-fr" in owcc is undocumented)

Again, don't mix unrelated change to single PR. I don't accept PR with such mixture of changes.

This pull request is still fixing the issue, that all RISC assembler do not work, when started through owcc.

I updated the title according the codechange of the latest patch.

Should i add a new pull request for the changes in the commandline help for "-oc" and "-oe", or is it ok to include it in the patch, where I added "-fr" to the commandline help?

jmalak commented 5 months ago

The -fr must works as specified, it has nothing to do with internal processing (implementation), because command line behaviour must be as specified.

To implementation, if there exists some -fr implementation and it is against this specification then it must be change to follow what is specified. When and how open/create error file is implementation that if some exists then should be used. But if it is against this specification then can be changed. You need study code to understand why it was done some way and next you can deside what exactly need change.

We have one simple rule, change only what is necesary, especialy if some backward compatibility must be hold.

jmalak commented 5 months ago

Assemblers must fully support syntax -fr[=<file name>] on command line by example it can skip file name, but must respect syntax

jmalak commented 5 months ago

wasm is fixed by a60f95424177e818205998922a2a55122d9850ce commit

jmalak commented 5 months ago

wasaxp -fr=blb.err

get

Open Watcom Alpha AXP Assembler Version 2.0 beta May 31 2024 11:20:25 (32-bit)
Copyright (c) 2002-2024 The Open Watcom Contributors. All Rights Reserved.
Portions Copyright (c) 1984-2002 Sybase, Inc. All Rights Reserved.
Source code is available under the Sybase Open Watcom Public License.
See https://github.com/open-watcom/open-watcom-v2#readme for details.
Error: invalid option -fr=blb.err

It is wrong

jmalak commented 5 months ago

I changed as parser to generated parser and fix -fr syntax. Please, rebase and fix your PR to latest source code.

winspool commented 5 months ago

wasaxp -fr=blb.err

get

Open Watcom Alpha AXP Assembler Version 2.0 beta May 31 2024 11:20:25 (32-bit)
Copyright (c) 2002-2024 The Open Watcom Contributors. All Rights Reserved.
Portions Copyright (c) 1984-2002 Sybase, Inc. All Rights Reserved.
Source code is available under the Sybase Open Watcom Public License.
See https://github.com/open-watcom/open-watcom-v2#readme for details.
Error: invalid option -fr=blb.err

It is wrong

That's strange, because everything worked here with my lates patch

commit 8b001f42bd75357788f14a157bf2f903163dfe40 (HEAD -> owcc)
Author: Detlef Riekenberg
Date:   Wed May 29 02:28:16 2024 +0200

    as: Implement argument "-fr" for the error report file

    This allows the RISC assemblers to work with owcc

    --
    Regards ... Detlef

:100644 100644 f399f52c40 2e5892741a M  bld/as/c/main.c
:100644 100644 800d99d1d4 af57a4eec1 M  bld/as/c/obj.c
:100644 100644 5324bc7440 32ca4820e8 M  bld/as/c/options.c
:100644 100644 9410dd5f63 1465b4798b M  bld/as/h/obj.h
:100644 100644 c80c7e7392 cd80f22684 M  bld/as/h/options.gml
:100644 100644 bb304fc479 4d71e72585 M  bld/as/h/options.h

commit c7736b75f25963d2681c8377951c4292b37e1aae (origin/master, origin/HEAD, master)

Which patch did you tested? Is it possible, that my last push did not work?

I even did a new "git format patch" today, and the saved patch has the same ID's for the changed files.

winspool commented 5 months ago

I changed as parser to generated parser and fix -fr syntax.

Thanks

Please, rebase and fix your PR to latest source code.

I will look at it tomorrow.

jmalak commented 5 months ago

I think the new flag is not necessary. I think simple solution exists (see C compiler or Wasm -fr solution). You predefine template to "*" and if -fr has no argument set template to NULL and if argument exists then duplicate it to template. If template is not NULL open error file by template otherwise not open error file.

winspool commented 5 months ago

In the meantime, "-fr" support was implemented in a different way: https://github.com/open-watcom/open-watcom-v2/commit/cad1d3181dd54303f2686ada5cda3d2353e46df9