This is an archive of the discontinued LLVM Phabricator instance.

[ms] [llvm-ml] Warn on command-line redefinition
ClosedPublic

Authored by epastor on Jun 9 2021, 9:57 PM.

Details

Summary

If a macro is defined on the command line and then overridden in the source code, this is likely to be an error in the user's build system. We should warn on this.

Diff Detail

Event Timeline

epastor created this revision.Jun 9 2021, 9:57 PM
epastor requested review of this revision.Jun 9 2021, 9:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2021, 9:57 PM
epastor added inline comments.
llvm/test/tools/llvm-ml/command_line_defines_errors.asm
1

I couldn't get lit to just detect the warning, for some reason. I'm open to suggestions!

thakis added inline comments.Jun 10 2021, 5:56 AM
llvm/test/tools/llvm-ml/command_line_defines.asm
1

Maybe you need 2>&1 in front of the | so that lit sees stderr?

1

Wait you meant the other test. One moment…

I tried patching this in

llvm/test/tools/llvm-ml/command_line_defines_errors.asm
1

Hm, works for me:

; RUN: llvm-ml -filetype=s %s /Fo - /Dtest1=def 2>&1 | FileCheck %s --implicit-check-not=warning:

.code

; CHECK: :[[# @LINE + 1]]:1: warning: redefining 'test1', already defined on the command line
test1 textequ <redef>

end
% out/gn/bin/llvm-lit -s -vv llvm/test/tools/llvm-ml/command_line_defines_errors.asm


Testing Time: 0.13s
  Passed: 1
epastor updated this revision to Diff 351152.Jun 10 2021, 6:14 AM

Directly check for the warning, rather than converting it to an error

epastor marked an inline comment as done.Jun 10 2021, 6:15 AM
epastor added inline comments.
llvm/test/tools/llvm-ml/command_line_defines_errors.asm
1

... darn, I would've sworn I tried that, but apparently I kept putting it in the other file - forgetting that CHECK always requires lines to occur in order, and that the warning is emitted before any of the output lines I'm checking against.

Thanks!

thakis accepted this revision.Jun 10 2021, 8:50 AM

LG with tweak below

llvm/test/tools/llvm-ml/command_line_defines_errors.asm
1

You don't need the implicit check not for error: if there's an error the command will exit non-0 and lit will fail the test

This revision is now accepted and ready to land.Jun 10 2021, 8:50 AM
epastor updated this revision to Diff 351212.Jun 10 2021, 10:27 AM
epastor marked an inline comment as done.

Dropping unnecessary implicit-check-not guard

This revision was automatically updated to reflect the committed changes.