Page MenuHomePhabricator

Lex: add support for `{,u}i128` Microsoft extension
Needs ReviewPublic

Authored by compnerd on Mar 11 2022, 3:03 PM.

Details

Summary

The Windows SDK has occurrences of the i128 and ui128 suffix (at
least in the 10.0.22000.0 SDK, see intsafe.h). This adds support for
the extension to properly lex the literal.

Diff Detail

Event Timeline

compnerd created this revision.Mar 11 2022, 3:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 3:03 PM
compnerd requested review of this revision.Mar 11 2022, 3:03 PM
rnk added a comment.Mar 15 2022, 10:47 AM

Seems reasonable, this could use far more testing.

clang/test/Lexer/ms-extensions.c
27

This seems like it could use more testing. Check for other __int128 tests, and try adding a few of these literals to those tests with some basic arithmetic expressions.

In a similar vein, I believe there are already bugs that lurk behind __int128 non-type template parameters. It's worth setting up a few of those test cases. One of the main usages of the expr printer is to produce debug info for non-type template parameters. Do i128 suffixes come through there correctly now?

The Windows SDK has occurrences of the i128 and ui128 suffix (at least in the 10.0.22000.0 SDK, see intsafe.h). This adds support for the extension to properly lex the literal.

Something fishy is still going on here. I definitely see the use in intsafe.h, but it seems that i128 is not supported by MSVC nor is __int128 as a type: https://godbolt.org/z/K36baejq6 in neither C nor C++ mode, at least not for any architecture I can find. Were you running into compile errors with clang-cl when compiling intsafe.h, or was this more speculative that since it's in the header it must be supported? I have the distinct impression this is a bug in the Windows SDK: https://godbolt.org/z/sTx8W4a51.

This was something that I was hitting an issue with. In particular, it was a module build for a module which pulled in intsafe.h. Now, given that it is a preprocessor macro, it would stand to reason that it will normally be dropped and thus won't matter if the frontend doesn't actually support a 128-bit type. However, with a module, we would attempt to process the expression. I do admit it is on a slightly more tenuous ground as Microsoft may somehow do something different.

This was something that I was hitting an issue with.

Does cl hit the same issue when building a module that includes intsafe.h?

In particular, it was a module build for a module which pulled in intsafe.h. Now, given that it is a preprocessor macro, it would stand to reason that it will normally be dropped and thus won't matter if the frontend doesn't actually support a 128-bit type. However, with a module, we would attempt to process the expression. I do admit it is on a slightly more tenuous ground as Microsoft may somehow do something different.

I am by no means a modules expert (so I'm adding folks to the review who are for their input), but I guess I am naively surprised that a module build would evaluate the macro's expansion list when no code in the module actually expands the macro. So I can't tell whether the issue is that Clang is emitting an error when it shouldn't, or whether cl.exe is failing to emit an error when it should, or something else. But it seems wrong to me to add a Microsoft extension that cl.exe doesn't actually support; that's likely to lead to a fair amount of user confusion. But it could also lead to ABI issues later if Microsoft did decide to support this type but using slightly different calling conventions than Clang uses.

iains added a comment.Mar 16 2022, 7:32 AM

could we see an example of the modular code that fails (and what flavour of modules compilation is being used)?
or is there already a PR for this?

I don't have an example module sadly. It was something that I ran into with Swift code import the WinSDK module defined in https://github.com/apple/swift/blob/main/stdlib/public/Platform/winsdk.modulemap.

iains added a subscriber: vsapsai.Mar 17 2022, 1:55 AM

hmm that makes it quite hard to analyse what's going on ...
.. however, it seems that this is a clang-modules style build (I am more familiar with the C++20 modules stuff, so probably @vsapsai will be able to throw more light on this).
however, I concur with @aaron.ballman that the original fail seems pretty fishy - at least for C++20 modules we do not do anything special in the pre-processor (with the exception of Header Units, but that code is not yet landed).

I don't have an example module sadly. It was something that I ran into with Swift code import the WinSDK module defined in https://github.com/apple/swift/blob/main/stdlib/public/Platform/winsdk.modulemap.

Could this be the reproducer? https://godbolt.org/z/YbbMse9a4

I don't know how "intsafe.h" looks like and how it is involved with the modules (it's not in winsdk.modulemap as far as I can tell). My immediate guess would be that "intsafe.h" doesn't include all the headers that are supposed to provide necessary macros. And if "intsafe.h" is in a separate module, for the code

#define SOME_MACRO 1
#include "intsafe.h"

"intsafe.h" won't see SOME_MACRO. That's how it can end up with different macro expansions. But that is just a guess (that can be waay wrong) and you could have checked this scenario already.

iains added a comment.Mar 18 2022, 4:38 AM

I don't have an example module sadly. It was something that I ran into with Swift code import the WinSDK module defined in https://github.com/apple/swift/blob/main/stdlib/public/Platform/winsdk.modulemap.

Could this be the reproducer? https://godbolt.org/z/YbbMse9a4

For C++20 modules, the basic rejection of this code is correct; we told the compiler it was a module but the source is not a valid module file (the first non-comment token needs to be a module/expoert decl).
The diagnostic is not especially friendly - but it does not seem to me that the example involves any attempt to expand the macro - it is perhaps poor recovery from the initial error.

So, I m not sure if we've explained the original problem yet - or whether other modules "flavours" would permit a non-module directive to precede any valid one (I suspect that modules-ts has a more flexible allowance, where there is an implicit global module fragment)

urnathan resigned from this revision.May 18 2022, 5:38 AM