We are migrating the bug tracker to github Issues. This is now the preferred way to report NASM bugs.

Self-registration is disabled due to spam issue (mail gorcunov@gmail.com or hpa@zytor.com to create an account)

Bug 3392420 - Incorrect dependency generation in -MD flag
Summary: Incorrect dependency generation in -MD flag
Status: RESOLVED FIXED
Alias: None
Product: NASM
Classification: Unclassified
Component: Assembler (show other bugs)
Version: 2.13.xx
Hardware: All All
: Medium normal
Assignee: nobody
URL:
Depends on:
Blocks:
 
Reported: 2017-08-08 08:55 PDT by kurosu
Modified: 2017-08-13 12:34 PDT (History)
5 users (show)

Obtained from: From OS distribution
Generated by: ---
Bug category:
Observed for: ---
Regression: ---
Regression since:


Attachments
Simplified example source code (318 bytes, application/zip)
2017-08-08 08:55 PDT, kurosu
Details

Note You need to log in before you can comment on or make changes to this bug.
Description kurosu 2017-08-08 08:55:22 PDT
Created attachment 411600 [details]
Simplified example source code

Platform is MSYS2 (no idea if patches were applied). Package "mingw64/mingw-w64-x86_64-nasm 2.13.01-1" is installed.

With the attached files (purposely reduced), running:
nasm -f win64 <OPTIONS> test.d -o /dev/null test2.asm && cat test.d
results in:
- with <OPTIONS>="-MD", "nul: test2.asm"
- with <OPTIONS>="-M -MF", "nul: test2.asm test1.asm"
Only the second output is valid.
Larger, more meaningful commands and source code are equally affected.

To summarize, -MD doesn't seem to properly output all dependencies, in particular those due to %include.
Comment 1 Martin Storsjö 2017-08-08 09:12:07 PDT
I sent a fix suggestion for this in https://sourceforge.net/p/nasm/mailman/message/35987075/.
Comment 2 kurosu 2017-08-08 09:27:33 PDT
In reply to the question in the mail of the posted patch, this is for a single pass generation rule, as used by 2 projects that Martin likely knows. :-)
Comment 3 Cyrill Gorcunov 2017-08-08 09:30:10 PDT
Will take a look, thanks!
Comment 4 Martin Storsjö 2017-08-08 09:41:41 PDT
(In reply to kurosu from comment #2)
> In reply to the question in the mail of the posted patch, this is for a
> single pass generation rule, as used by 2 projects that Martin likely knows.
> :-)

What I meant about that is that the assembler internally runs a number of passes. The bug lies in the fact that it would have worked if it would require less internal passes. It only records the filenames for the dependency list on the last run - but include filename lookups are cached, and if cached, aren't added to the dependency list.

Thinking of it now, I'm not sure if this fix is right, in case the same file is included multiple times, or if we instead should clear the hash between passes. That might give some performance hit though.
Comment 5 Martin Storsjö 2017-08-08 10:40:50 PDT
(In reply to Martin Storsjö from comment #4)
> Thinking of it now, I'm not sure if this fix is right, in case the same file
> is included multiple times, or if we instead should clear the hash between
> passes. That might give some performance hit though.

Scratch that - the string list does deduplication.
Comment 6 H. Peter Anvin 2017-08-08 13:42:56 PDT
I can definitely confirm this bug.
Comment 7 kurosu 2017-08-08 14:23:50 PDT
Is this OS-dependent? Proposed patch does not hint at that. I find it strange that no user of the aforementioned projects have never noticed the issue.
Comment 8 kurosu 2017-08-08 14:53:01 PDT
(In reply to kurosu from comment #7)
> Is this OS-dependent? Proposed patch does not hint at that. I find it
> strange that no user of the aforementioned projects have never noticed the
> issue.

Confirmed to happen elsewhere, even if it seemed obvious.
Comment 9 Martin Storsjö 2017-08-08 14:56:04 PDT
(In reply to kurosu from comment #8)
> (In reply to kurosu from comment #7)
> > Is this OS-dependent? Proposed patch does not hint at that. I find it
> > strange that no user of the aforementioned projects have never noticed the
> > issue.
> 
> Confirmed to happen elsewhere, even if it seemed obvious.

I've heard it mentioned before, but not had the time to look into it before. It was changed this way pretty recently (some months ago?) - previously yasm was used by default, and then the dependencies were generated by running the assembler twice.
Comment 10 Cyrill Gorcunov 2017-08-08 15:15:32 PDT
Guys, I fear I won't be able to look precisely on the issue until the weekend, or at least after a couple of days. So if it's urgent and the patch fixes the issue, Peter mind to take it so we revisit it a bit later?
Comment 11 kurosu 2017-08-08 16:50:45 PDT
I'd say it's not urgent. Likely, a version check would be needed once a release is made with the fix. Therefore, availability of an updated package would require even longer.
Comment 12 Cyrill Gorcunov 2017-08-13 12:34:43 PDT
Fixed in 1fdc5f001c06fdfa04a3ab4a9b9bd002dc591c10