In D121122#3402442, @aaron.ballman wrote:

In D121122#3402334, @bjope wrote:

Hello. We've got some problem in our downstream tests after this patch and I'm trying to figure out how things are supposed to work. Maybe someone being part of this review knows.

Sorry for the troubles!

Problem is that we have some libc verification suites that include test cases using float_t. But in math.h from for example newlib the type float_t isn't defined if FLT_EVAL_METHOD is set to -1.
The standard says that float_t is implementation defined when FLT_EVAL_METHOD isn't 0/1/2. But I'm not quite sure how that is supposed to be handled (such as where to define float_t if not in math.h).

The way I read the requirements in C2x 7.12p3 are that the types float_t and double_t are always defined in <math.h>, but in the event FLT_EVAL_METHOD isn't 0, 1, or 2, the implementation has to pick whatever type makes the most sense for the underlying types.

One question is: If I have a piece of C99 code using float_t, is that code not allowed to be compiled using fast-math?

The standard doesn't admit that fast math is a thing; it's up to the implementations to define what their fast math extension does.

I guess it should be seen as with fast-math the float_t precision is unknown. But the type still has to be defined somewhere or else the frontend will complain about "unknown type name". But I'm not sure where this is supposed to be handled.

I think it should still be handled in <math.h>. *I am not a C floating point expert, so take this suggestion with a grain of salt*: I think it is defensible to fallback to defining float_t as float and double_t as double when the eval method is indeterminable. @andrew.w.kaylor may have more nuanced thoughts here.

In D118837#3395529, @Fznamznon wrote:

Was this one submitted?

@MaskRay Thanks. Will do that.

Thanks @aaron.ballman

@majnemer Review please :)

@majnemer Any feedback please? thanks.

In D114483#3281731, @aaron.ballman wrote:

In D114483#3281718, @aaron.ballman wrote:

In D114483#3281714, @zahiraam wrote:

In D114483#3280905, @MaskRay wrote:

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

The issues aren't from clang-format, they're from the RST file contents not building in sphinx due to formatting issues in AttrDocs.td.

https://lab.llvm.org/buildbot/#/builders/92/builds/20791/steps/5/logs/stdio
https://lab.llvm.org/buildbot/#/builders/92/builds/21098/steps/5/logs/stdio

are two such examples that stem from your patch. Basically, you have to build the attribute documentation (converting AttrDocs.td into AttributeReference.rst) and then build the sphinx documents locally to ensure you're not introducing new issues. Or, you can try to speculatively fix based on the errors the bot gives you, but sometimes those errors are super cryptic and take multiple tries to fix (like the "can't lex as C++" one above), which is why trying to reproduce locally is a good first step.

I've pushed a fix for it in a10ff373ddfa82a67c580a87f6258aa6ab8dd595 and the sphinx builder is happy again.

In D114483#3280905, @MaskRay wrote:

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

Thanks for the reviews.

In D117569#3262595, @zahiraam wrote:

@majnemer

I did take an example completely different with no use of the constexpr in order to understand (at least for me) when the _imp prefix is generated with CL.
This is sample.cpp:
#include<iostream>

__declspec(dllexport) void PrintHello()
{

std::cout << "hello" << std::endl;

}

$ cl sample.cpp /LD /EHsc
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27045 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

sample.cpp
Microsoft (R) Incremental Linker Version 14.16.27045.0
Copyright (C) Microsoft Corporation. All rights reserved.

/out:sample.dll
/dll
/implib:sample.lib
sample.obj

Creating library sample.lib and object sample.exp

$
This is testapp.cpp:
extern void __declspec(dllimport)PrintHello();

void main()
{

PrintHello();

}

$ cl testapp.cpp /EHsc /link sample.lib
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27045 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

testapp.cpp
Microsoft (R) Incremental Linker Version 14.16.27045.0
Copyright (C) Microsoft Corporation. All rights reserved.

/out:testapp.exe
sample.lib
testapp.obj
$
$ dumpbin /symbols testapp.obj | grep imp
008 00000000 UNDEF notype External | imp_?PrintHello@@YAXXZ (declspec(dllimport) void __cdecl PrintHello(void))
$

My understanding that the _imp prefix is generated because of the dllimport in testapp.cpp.
So if we go back to the test case above:

extern int __declspec(dllimport) dll_import_int;
constexpr int& dll_import_constexpr_ref = dll_import_int;
int& get() {

return dll_import_constexpr_ref;

}
int main () {

get();
return 0;

}

CLANG (with this patch and ICL for that matter) generates the symbol:

011 00000000 UNDEF notype External | imp_?dll_import_int@@3HA (declspec(dllimport) int dll_import_int)

but not MSVC. This seems incorrect to me since the _imp prefix is always generated for dllimport symbols?

I did take an example completely different with no use of the constexpr in order to understand (at least for me) when the _imp prefix is generated with CL.
This is sample.cpp:
#include<iostream>

In D117569#3259928, @majnemer wrote:

Your example is different from mine as it nests the constexpr variable inside the function rather than having it at translation-unit scope.

In D117569#3259255, @majnemer wrote:

In D117569#3258307, @zahiraam wrote:

In D117569#3257121, @majnemer wrote:

I have a question regarding how this work with respect to the dllimport semantics known by the linker.
IIUC, we will now allow a program like:

extern int __declspec(dllimport) dll_import_int;
constexpr int& dll_import_constexpr_ref = dll_import_int;
int& get() {
    return dll_import_constexpr_ref;
}

Here, get will load dll_import_constexpr_ref. However, what will dll_import_constexpr_ref hold? It ought to hold the contents of __imp_?dll_import_int@@3HA. However, we can't dereference __imp_?dll_import_int@@3HA to get to its contents.

@majnemer Thanks for the review.

This test case doesn't link with MSVC. It will generate this error:
Microsoft (R) Incremental Linker Version 14.29.30133.0
Copyright (C) Microsoft Corporation. All rights reserved.

/out:test1.exe
test1.obj
test1.obj : error LNK2001: unresolved external symbol "int dll_import_int" (?dll_import_int@@3HA)
test1.exe : fatal error LNK1120: 1 unresolved externals

The symbols generated with MSVC are:
07 00000000 UNDEF notype External | ?dll_import_int@@3HA (int dll_import_int)
015 00000000 SECT6 notype Static | ?dll_import_constexpr_ref@@3AEAHEA (int & dll_import_constexpr_ref)

Without this patch this test case errors. With this patch clang will generate these symbols:

010 00000000 UNDEF notype External | imp_?dll_import_int@@3HA (declspec(dllimport) int dll_import_int)
012 00000000 SECT5 notype External | ?dll_import_constexpr_ref@@3AEAHEA (int & dll_import_constexpr_ref)
013 00000000 UNDEF notype External | ?dll_import_int@@3HA (int dll_import_int)

and will error at link time with this error:
test1-f1f63b.o : error LNK2019: unresolved external symbol "declspec(dllimport) int dll_import_int" (imp_?dll_import_int@@3HA) referenced in function "int & __cdecl get(void)" (?get@@YAAEAHXZ)
test1-f1f63b.o : error LNK2001: unresolved external symbol "int dll_import_int" (?dll_import_int@@3HA)
a.exe : fatal error LNK1120: 2 unresolved externals

I think that's the behavior expected, right?

My interpretation is that MSVC has a bug: it is forming a reference to ?dll_import_int@@3HA and not __imp_?dll_import_int@@3HA. Could you run a more complete experiment where you have a dll which exports the symbol and you try to import it?

In D117569#3257121, @majnemer wrote:

I have a question regarding how this work with respect to the dllimport semantics known by the linker.
IIUC, we will now allow a program like:

extern int __declspec(dllimport) dll_import_int;
constexpr int& dll_import_constexpr_ref = dll_import_int;
int& get() {
    return dll_import_constexpr_ref;
}

Here, get will load dll_import_constexpr_ref. However, what will dll_import_constexpr_ref hold? It ought to hold the contents of __imp_?dll_import_int@@3HA. However, we can't dereference __imp_?dll_import_int@@3HA to get to its contents.

@Fznamznon @bader @Naghasan reviews please? Thanks.

This change is generating this crash discussed here: https://bugs.llvm.org/show_bug.cgi?id=49834
@aaronpuchert do you have any fix for it?