ping?

ping?

mark dso_local only when non-pic

In D108421#2958848, @MaskRay wrote:

If you read the comment in TargetMachine::shouldAssumeDSOLocal: this is the wrong direction. dso_local is assumed to be marked by the frontend.

Direct accesses and GOT accesses are trade-offs. Direct accesses are not always preferred. The MachO special case is an unfortunate case for their xnu kernel, not a good example here.

updated test and make changes local to auto generated global vars for lock.

In D108421#2958745, @lebedev.ri wrote:

In D108421#2958742, @kamleshbhalui wrote:

assume dso local if relocation model static

I think these are two separate fixes. TargetMachine::shouldAssumeDSOLocal() change might make sense,
but as the comment there notes, this really should be fixed in in CGOpenMPRuntime::getOrCreateInternalVariable() itself (is that not the right default?),
and in it's every user that incorrectly unmarks said global as being `dso_local.

assume dso local if relocation model static

updated test and make changes local to auto generated global vars for lock.

clang formatted

In D103131#2835909, @dblaikie wrote:

In D103131#2835702, @kamleshbhalui wrote:

Here is what we get when we replace int with float.

$lldb-11 ./a.out 
(lldb) target create "./a.out"
Current executable set to '/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/a.out' (x86_64).
(lldb) b main
Breakpoint 1: where = a.out`main + 4 at test.c:3:12, address = 0x0000000000400484
(lldb) p oldname
(float) $0 = 1
(lldb) p newname
(void *) $1 = 0x000000003f800000

Yep, looks like it's ignoring the imported declaration in favor of the raw symbol table entry.

How's lldb handle GCC-style debug info (the variable declaration, without any imported declaration) for the alias?

Here is what we get when we replace int with float.

In D103131#2834744, @dblaikie wrote:

Huh, that surprises me - guess gdb favors checking the symbol first. I guess maybe it is using something that determines that that symbol comes from the file with debug info - because on a similar test case (one file without debug info, defining some global variable i, another file with debug info with a using ns::i in the global scope - printing i when stepping into the second file correctly prints the using based alias value, but stepping into the file without debug info and printing i complains about not knowing the type of that i)

How's lldb go?

In D103131#2815386, @probinson wrote:

0x0000002a:   DW_TAG_variable
                DW_AT_name      ("oldname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (1)
                DW_AT_location  (DW_OP_addr 0x0)

0x0000003f:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000046:   DW_TAG_variable
                DW_AT_name      ("newname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_declaration       (true)

0x00000051:   DW_TAG_imported_declaration
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_import    (0x00000046)
                DW_AT_name      ("newname")

I agree with David, this sequence doesn't seem to do what's desired.
There's nothing that ties "newname" to "oldname" here. What you
want is something more like this:

0x0000002a: DW_TAG_variable
               DW_AT_name ("oldname")
               ...
0x0000003a: DW_TAG_imported_declaration
               DW_AT_import (0x0000002a)
               DW_AT_name ("newname")

That is, there would not be a separate DW_TAG_variable for "newname";
instead, the imported_declaration would import the DIE for "oldname"
giving it the name "newname".

--paulr

In D103131#2814015, @dblaikie wrote:

In D103131#2811969, @kamleshbhalui wrote:

In D103131#2811220, @dblaikie wrote:

Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.

But yeah, at the moment I don't have any great reason to avoid the imported declaration form - so happy to go with that.

Hi David,

with imported declaration patch and with current patch(in review or say gcc way) this case works ok(we can print type and value of newname)
$cat test.c
int oldname = 1;
extern int newname attribute((alias("oldname")));

but when we make newname static it works with current patch(in review or say gcc way) but it does not work with imported decl patch(https://reviews.llvm.org/D103131?id=347883).

Should we go with gcc way or am I missing something?
note: used gdb debugger.

An ideas what's happening when newname is static? Is the DWARF any different/interesting there? (since the DWARF for imported decl seems like it would have nothing to do with the linkange of the alias - since it doesn't refer to the actual alias in the object file, etc)

In D103131#2811220, @dblaikie wrote:

Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.

But yeah, at the moment I don't have any great reason to avoid the imported declaration form - so happy to go with that.

In D103131#2791881, @dblaikie wrote:

In D103131#2789493, @probinson wrote:

Mixed feelings - somewhat in favor of "do the thing that's probably already fairly tested/known to work" (GCC's thing). But open to the idea that that approach has problems, for sure.

"Known to work" for whom? gdb, sure, because gcc/gdb cooperate on many things.

Yep. Given the diversity of ways of expressing things in DWARF, no consumer will handle everything that could be produced in the way a producer might intend - so we do sort of have a defacto standard of "what would GCC do/what does GDB understand".

But also GCC/GDB has had more implementation experience with DWARF in general, and with any feature we haven't implemented yet in particular - that I wouldn't throw out their representational choice too quickly - there may be important tradeoffs that were considered, implemented, and discarded due to limitations.

I'll have to check with my debugger guys whether this would work for us; and I'll just reiterate that it's certainly not what the DWARF spec suggests should be done.

I don't agree that it's "certainly not what the DWARF spec suggests should be done" - the spec's pretty generous and exactly how a C or ELF level alias should be rendered in DWARF seems pretty unclear to me. For instance an alias is a symbol in the ELF file, arguably different from a source level alias like a typedef or using declaration that DW_TAG_imported_declaration seems to be promoted for.

I doubt gdb would have trouble with DW_TAG_imported_declaration

In D103131#2791373, @probinson wrote:

In D103131#2789493, @probinson wrote:

Mixed feelings - somewhat in favor of "do the thing that's probably already fairly tested/known to work" (GCC's thing). But open to the idea that that approach has problems, for sure.

"Known to work" for whom? gdb, sure, because gcc/gdb cooperate on many things. I'll have to check with my debugger guys whether this would work for us; and I'll just reiterate that it's certainly not what the DWARF spec suggests should be done.

The Sony debugger will not be able to figure out the address of "newname" given the DWARF as emitted by gcc (because it looks like an external declaration with no address). We'd much rather have the DW_TAG_imported_declaration that the original patch had.

Good to know - any interest in the debugger supporting declarations without addresses? I guess there's no need for GCC compatibility? Clang might emit them under some circumstances... let's see... Hmm, can't find a case now. But there are cases where it would be desirable (such as when compiling some code at -g0 and some with debug info - when you only have the declaration on the debug info side, and no debug info on the definition side (eg: GCC produces a declaration of 'i' in this code: extern int i; int main() { return i; }))

@kamleshbhalui - perhaps you could run the gdb and lldb test suites without either change, then with both the variable declaration and imported declaration versions to see how the results vary? (Well, I guess the lldb test suite probably won't show any changes - but maybe worth running anyway) - along with the manual test you've described in the patch description, to demonstrate that both solutions address that?

$cat test.h
int oldname;

matching gcc behavior

match gcc behavior

removed unnecessary code.

In D103131#2780997, @dblaikie wrote:

Looks like GCC emits aliases as a DW_TAG_variable without a location, not as a DW_TAG_imported_declaration - what gave you the inspiration to do it in this way? (I think it's probably good, but DWARF doesn't lend itself to novelty so much... can be good to stick with how other folks already do things since it'll be what consumers already know how to handle)

How's this work if the alias target isn't declared in the source - but in inline assembly instead? (I guess GCC probably handles that OK, but the Clang strategy here might not cope with it)

updated generate_feature_test_macro_components.py.

In D94511#2698813, @Mordante wrote:

There are still quite some formatting errors, for example () , instead of (),, please use the instructions here to format the patch https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

formatted intcmp.fail.cpp

formatted the tests as per @Quuxplusone comments

incorporated @Quuxplusone comments

fixed a typo.

incorporated comments from @Quuxplusone, @Mordante and @curdeius

Making changes effective only for windows

ping?

Incorporated @Quuxplusone suggestions.

Addressed @curdeius comments.

Making changes effective only for windows

updated patch

clang formatting the patch.

Marked failed tests as unsupported on windows system
Because now clang emits native path for DIFile.

In D99580#2659352, @dblaikie wrote:

@rnk @akhuang - I think we touched on this before maybe in some other thread/patch/discussion?

Ah, @probinson mentioned in the linked thread:

Is this https://bugs.llvm.org/show_bug.cgi?id=44170 which had a tentative patch at https://reviews.llvm.org/D71508 ?
The original complaint wasn't for Windows, but the lack of filepath canonicalization seems like a common symptom.

Might be best, @kamleshbhalui, if you could check if the other (D71508) patch relates to the same issue & if so, continue the discussion over there instead of forking it here.

In D99580#2659858, @amccarth wrote:

Another possible issue is that llvm::sys::path and other functions try to map Windows filesystem concepts to Posix ones. There are many cases where there isn't a perfect mapping. For example: Windows has a current working directory _per drive_, and so it can be hard to resolve paths like D:foo.ext, which are relative to something other than the "current working directory." Details like this can come into play in the immediate vicinity of the code change, so I have some trepidation.

Agree with you but this problem exist with and without this patch.

It looks like the code change is for everyone, but the new test is specific to mingw.

Updated the tests

In D98006#2606717, @curdeius wrote:

This doesn't answer my question unfortunately.
What configuration (platform/OS/compiler, cmake invocation etc.) you used when you found the failures?
What were these failures precisely?

In D98006#2606418, @curdeius wrote:

Can you give some details please about the config on which these tests failed?

Added one fixed test

Fixed more tests

addressed @Mordante comments.

used _LIBCPP_NO_HAS_CHAR8_T and _LIBCPP_HAS_NO_UNICODE_CHARS macro to guard char8_t, and char16_t, char32_t.

addressed @curdeius comments.
added tests, few test snippets taken from the link given by @curdeius.

fixed a nit.

rebased with the main branch. do not see any conflict.
@ldionne or @abdulras can you please have a look at this, this patch is in this state for a long time.
FYI, I have been using this patch in VxWorks.

LGTM!

In D88727#2330286, @mclow.lists wrote:

Is this a problem unique to vector? Do the other containers have the same constructor? Do those constructors have the same issue?
Checking ... list does not seem to have this problem. deque does not either. forward_list does not either.

<deque> has the following code:

template <class _InputIter>
    deque(_InputIter __f, _InputIter __l,
          typename enable_if<__is_cpp17_input_iterator<_InputIter>::value>::type* = 0);
template <class _InputIter>
    deque(_InputIter __f, _InputIter __l, const allocator_type& __a,
          typename enable_if<__is_cpp17_input_iterator<_InputIter>::value>::type* = 0);

I don't think that making vector different from the other containers is the right approach. Then again, I'm not 100% sure why the existing code is failing - so I should look at tha

ping?

Added test suggested by @mclow.lists .

In D88727#2308363, @miscco wrote:

So the actual problem in the bug report is that the first argument cannot be used to deduce the type.

I think it is not correct to remove the type restrictions here, they are there for a reason.

LGTM!

rebased with the master.

addressed @mhorne comments.

@asb and @luismarques do you suggest anything here ?

ping?

ping?

addressed @mhorne and @MaskRay concerns.

rebased and cleaned up the diff.

In D82171#2103042, @tambre wrote:

D77505 broke a bunch of bots. This fixes the regression.

I lack commit privileges. Please land as "Raul Tambre <raul@tambre.ee>"

In D77505#2102613, @tambre wrote:

Please commit this for me in case no one else has objections. I lack commit privileges.

Addressed @efriedma comments.
added test for strictfp.