This is an archive of the discontinued LLVM Phabricator instance.

[Lex] Diagnose macro in command lines
Needs ReviewPublic

Authored by jackhong12 on Jul 3 2022, 11:59 PM.

Details

Summary

Description

We need to diagnose the macro in commands too. For example, the expected result of clang++ -Wreserved-identifier -D__WHY_NOT_ME__ -U__STDC__ tmp.cpp should be

<command line>:1:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
#define __WHY_NOT_ME__ 1
        ^
<command line>:2:8: warning: macro name is a reserved identifier [-Wreserved-macro-identifier]
#undef __STDC__
       ^

Currently, clang will not show any warning message when we define or undefine reserved macro in commands.

Fix

We use digital directives in the preprocessor, like

# 1 "<built-in>" 3
#define __llvm__ 1
#define __clang__ 1

...

# 1 "<command line>" 1
#define __WHY_NOT_ME__ 1
#undef __STDC__
#define __GCC_HAVE_DWARF2_CFI_ASM 1
# 1 "<built-in>" 2

I think we should use PresumedLoc to determine its <built-in> or <command line> section. In this case, the file name will be changed from <built-in> to <command line>. However, SourceMgr.getBufferName only returns the first name it matches.

Another issue is that clang will define __GCC_HAVE_DWARF2_CFI_ASM in the command line. But I don't know how to handle this macro. I only add this macro to the reserved macro list.

Diff Detail

Event Timeline

jackhong12 created this revision.Jul 3 2022, 11:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 11:59 PM
jackhong12 requested review of this revision.Jul 3 2022, 11:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 11:59 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Sorry, I didn't consider some cases. I'll fix it soon!

Thank you for looking into this! I think we may need to figure out something better for handling macros defined by the driver, because otherwise we're going to get a bunch of false positives from this. For example, the clang-cl driver adds command line flags like -D_DEBUG and -D_MT, etc, the same as what happens for __GCC_HAVE_DWARF2_CFI_ASM. I'm not certain if there's an easy way to differentiate between driver-added -D flags and user-added -D flags, though. If we don't have anything that suffices already, we could invent something (either a new form of -D at the cc1 level that the driver emits for its defines, or some sort of cc1 flag to say "here's where user command line options begin", etc).

great addition. Missing a test case though, which would probably trigger @aaron.ballman scenario

I added two flags, -driver-define and -driver-undefine, to indicate macros that the driver defines. And I moved driver-defined macros from <command line> file to <built-in> file, like

# 1 "<built-in>" 3
#define __llvm__ 1
#define __clang__ 1
...
# 1 "<command line>" 1
#define __WHY_NOT_ME__ 1
#undef __STDC__
# 1 "<built-in>" 2
#define __GCC_HAVE_DWARF2_CFI_ASM 1
...
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Jul 9 2022, 11:21 PM
clang/include/clang/Driver/Options.td
664 ↗(On Diff #443447)

Make this CC1 only option NoDriverOption by moving it somewhere under let Flags = [CC1Option, NoDriverOption] in {

Don't add new Separate or JoinedOrSeparate options. They are legacy.

  • modify option names
  • only allow driver-defined macros used in cc1
jackhong12 added inline comments.Jul 10 2022, 10:15 AM
clang/include/clang/Driver/Options.td
664 ↗(On Diff #443447)

I tried to add = behind driver-define. But I got the following error messages when I ran ninja check-all.

[416/433] cd /home/zhenhong/ssd/zhenhong/llvm-project/clang/bindings/python && /usr/bin/cmake -E env CLANG_LIBRARY_PATH=/home/zhenhong/ssd/zhenhong/llvm-project/release/lib /usr/bin/python3.8 -m unittest discover                          
FAILED: tools/clang/bindings/python/tests/CMakeFiles/check-clang-python                                                                                                                                                                       
cd /home/zhenhong/ssd/zhenhong/llvm-project/clang/bindings/python && /usr/bin/cmake -E env CLANG_LIBRARY_PATH=/home/zhenhong/ssd/zhenhong/llvm-project/release/lib /usr/bin/python3.8 -m unittest discover                                    
E..........EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE...EEEEEEEEEE.EEEEE......EEEE..EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE                                                                                                                
======================================================================                                                                                                                                                                        
ERROR: test_access_specifiers (tests.cindex.test_access_specifiers.TestAccessSpecifiers)                                                                                                                                                      
Ensure that C++ access specifiers are available on cursors                                                                                                                                                                                    
----------------------------------------------------------------------                                                                                                                                                                        
Traceback (most recent call last):                                                                                                                                                                                                            
  File "/home/zhenhong/ssd/zhenhong/llvm-project/clang/bindings/python/tests/cindex/test_access_specifiers.py", line 20, in test_access_specifiers                                                                                            
    tu = get_tu("""                                                                                                                                                                                                                           
  File "/home/zhenhong/ssd/zhenhong/llvm-project/clang/bindings/python/tests/cindex/util.py", line 40, in get_tu                                                                                                                              
    return TranslationUnit.from_source(name, args, unsaved_files=[(name,                                                                                                                                                                      
  File "/home/zhenhong/ssd/zhenhong/llvm-project/clang/bindings/python/clang/cindex.py", line 2837, in from_source                                                                                                                            
    raise TranslationUnitLoadError("Error parsing translation unit.")                                                                                                                                                                         
clang.cindex.TranslationUnitLoadError: Error parsing translation unit.

...

I don't know what triggers the crash.

This review went cold for about a year now (sorry for not noticing we hadn't answered your question!), so I'm uncertain if @jackhong12 is still interested in pursuing this. If not, that's totally fine, just let us know so we can remove you as the assignee on the issue (https://github.com/llvm/llvm-project/issues/56159).

clang/include/clang/Driver/Options.td
664 ↗(On Diff #443447)

I've never seen output like that before -- were you able to resolve this crash? @MaskRay do you have ideas?

MaskRay added inline comments.Aug 16 2023, 4:56 PM
clang/include/clang/Driver/Options.td
664 ↗(On Diff #443447)

I suspect that this is a config issue.

This patch now requires some rebase to work. For example, the Flags/CC1Option mechanism has been replaced with Visibility<[CC1Option]>,