Skip to content
This repository was archived by the owner on Jul 8, 2022. It is now read-only.

Conversation

@mliszcz
Copy link
Collaborator

@mliszcz mliszcz commented Jul 13, 2020

This PR adds code location information to log messages and origin field in DevFailed exception.

Example log message:

1594734929 [139911612307200] DEBUG dserver/DevTest/test (/home/michal/Documents/cppTango/cppapi/server/subdev_diag.cpp:80) SubDevDiag::set_associated_device() entering ...
1594734929 [139911612307200] DEBUG dserver/DevTest/test (/home/michal/Documents/cppTango/cppapi/server/device_3.cpp:1881) Leaving Device_3Impl::write_attributes_34

Example exception (Except::print_exception output):

30: Tango NamedDevFailedList exception
30:    Exception for object Long64_attr_rw
30:    Index of object in call (starting at 0) = 0
30:        Severity = ERROR
30:        Error reason = DevTest_WriteAttrHardware
30:        Desc : DevFailed from write_attr_hardware
30:        Origin : virtual void DevTest::write_attr_hardware(std::vector<long int>&) at (/home/michal/Documents/cppTango/cpp_test_suite/cpp_test_ds/DevTest.cpp:1244)

The log message is updated automatically if you use macros like ERROR_STREAM or cout5.

To have exception origin field filled automatically, please use TANGO_THROW_EXCEPTION(reason, desc) macro (there is also TANGO_RETHROW_EXCEPTION and corresponding macros for _API_EXCEPTION).

See also: #564 (comment)

@mliszcz mliszcz force-pushed the detect-function-name branch 2 times, most recently from d1ee2d1 to 23f2544 Compare July 14, 2020 08:20
mliszcz added a commit to mliszcz/cppTango that referenced this pull request Jul 14, 2020
Done automatically with script published in:
tango-controls#742
mliszcz added 11 commits July 14, 2020 15:28
What we had was broken as if one of the strings was empty the other one
was truncated to 0 size as well.
Due to broken comparator implementation (see previous commit), this test
was never correct.
The log messages now contain file name and line number. Comparator must
be extended to support this scenario.
Done automatically with sed. See script published in:
tango-controls#742
Done automatically with script published in:
tango-controls#742
@mliszcz mliszcz force-pushed the detect-function-name branch from bd453fe to 63eaac7 Compare July 14, 2020 13:34
@mliszcz
Copy link
Collaborator Author

mliszcz commented Jul 14, 2020

Large commit "Define constants for API exception reasons" (23bba98) was done mostly automatically:

rm cppapi/server/tango_const.h.in
for e in `rg '"API_.*?"' -o -I -N | sort | uniq | sed 's/.$//' | cut -c 2-`; do
  find . -name '*.cpp' -o -name '*.tpp' -o -name '*.h' | xargs -I{} sed -i 's|"'$e'"|'$e'|g' {}
done
git checkout cppapi/server/tango_const.h.in
rg 'API_[A-Z][a-z][a-zA-Z]+' -o -I -N | sort | uniq | xargs -I{} echo 'const char* const {} = "{}";'
# replace definitions in tango_const.h.in with output of the above command
# fix broken compilation where needed

@mliszcz
Copy link
Collaborator Author

mliszcz commented Jul 14, 2020

Large commit "Replace Except::throw_exception with macro" (63eaac7) was done mostly automatically with following scripts:

for f in `rg -l -e '::throw_exception' -e '::re_throw_exception'`; do
  echo "patching $f"
  python ../rename.py $f > $f.tmp
  mv $f.tmp $f
done

# to remove extra newlines (forgot about that in rename script
for f in `rg TANGO_THROW_ -l`; do sed -i -e '/TANGO_THROW_/{n;d}' $f; done
# same for RETHROW

Changes to these were reverted:

cppapi/client/helpers/TangoExceptionsHelper.h
cppapi/server/except.cpp
cppapi/server/except.h
cppapi/client/apiexcept.h
cpp_test_suite/cpp_test_ds/SigThrow.cpp
cppapi/client/helpers/PogoHelper.h

Changes in these required manual corrections:

cppapi/client/devapi.h
patching cppapi/server/attrprop.h
cppapi/client/helpers/DeviceProxyHelper.h
cppapi/server/device.cpp
cppapi/server/dserver.cpp
cppapi/server/pollext.h
cppapi/server/w_attrsetval.tpp
cppapi/server/attribute.h
cppapi/server/pipe.h
cppapi/client/devapi_base.cpp
cppapi/client/dbapi_base.cpp
cppapi/server/attribute.cpp

Rename script:

import re
import sys

SEARCH_THROW = '::throw_exception'
SEARCH_RETHROW = '::re_throw_exception'

FLUSH = object()

def Parser():
    buff = ""
    while True:
        c = yield
        if (c is FLUSH):
            print(buff, end='')
            buff = ""
            continue

        buff += c

        if c.isspace():
            print(buff, end='')
            buff = ""
            continue

        if buff.endswith(SEARCH_THROW) or buff.endswith(SEARCH_RETHROW):

            api_obj, _method = buff.rsplit("::", maxsplit=1)
            api = not bool(re.match(r"^(::)?(Tango)?(::)?Except", api_obj))
            rethrow = buff.endswith(SEARCH_RETHROW)
            buff = ""

            arg_buff = ""
            args = []
            yield # opening brace
            while True:
                c = yield
                if c == ";":
                    args.append(arg_buff[:-1]) # closing brace
                    break
                if c == ",":
                    args.append(arg_buff)
                    arg_buff = ""
                    continue
                if c == '"':
                    arg_buff += c
                    while True:
                        c = yield
                        arg_buff += c
                        if c == '"':
                            break
                    continue
                arg_buff += c

            args = [s.replace("(const char *)", "") for s in args]
            args = [s.replace("(const char*)", "") for s in args]
            args = [s.strip() for s in args]

            if rethrow:
                assert len(args) == 4
                if api:
                    print("TANGO_RETHROW_API_EXCEPTION({}, {}, {}, {});".format(
                        api_obj, args[0], args[1], args[2]))
                else:
                    print("TANGO_RETHROW_EXCEPTION({}, {}, {});".format(
                        args[0], args[1], args[2]))
            else:
                assert len(args) == 3
                if api:
                    print("TANGO_THROW_API_EXCEPTION({}, {}, {});".format(
                        api_obj, args[0], args[1]))
                else:
                    print("TANGO_THROW_EXCEPTION({}, {});".format(
                        args[0], args[1]))

parser = Parser()
parser.send(None)

with open(sys.argv[1]) as file:
    for line in file:
        for char in line:
            parser.send(char)
    parser.send(FLUSH)

@mliszcz mliszcz requested review from bourtemb and t-b July 14, 2020 14:19
@mliszcz mliszcz marked this pull request as ready for review July 14, 2020 14:20
@mliszcz
Copy link
Collaborator Author

mliszcz commented Jul 15, 2020

Hi @t-b and @bourtemb, the build is passed and PR is ready for review. There are two lengthy commits generated with a script (attached above for reference). I suggest you to just pick some random files and check that, because the whole diff is huge.

  • "Define constants for API exception reasons" (23bba98)
  • "Replace Except::throw_exception with macro" (63eaac7)

Copy link
Collaborator

@t-b t-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice addition.

Once that is merged, we can close #366 IMHO.

Two small changes would be nice:

f3fa387 (Add macro for detecting current function name, 2020-07-11)

Can you add a #undef BOOST_CURRENT_FUNCTION so that we don't have that name floating around?

b79d92d (Include current file and line number in log messages, 2020-07-11)

Can you add

#undef TANGO_QUOTE
#undef TANGO_QUOTE_

to the bottom of tango_current_function.h.

@mliszcz
Copy link
Collaborator Author

mliszcz commented Aug 13, 2020

Can you add a #undef BOOST_CURRENT_FUNCTION so that we don't have that name floating around?

Can you add

#undef TANGO_QUOTE
#undef TANGO_QUOTE_

to the bottom of tango_current_function.h.

It's not that simple. Seems like macros are expanded lazily. See: https://gcc.gnu.org/onlinedocs/cpp/Argument-Prescan.html#Argument-Prescan

After substitution, the entire macro body, including the substituted arguments, is scanned again for macros to be expanded.

#define A 1
#define B A
#undef A

int foo() { return B; }
<source>: In function 'int foo()':
<source>:2:11: error: 'A' was not declared in this scope
    2 | #define B A
      |           ^
<source>:5:20: note: in expansion of macro 'B'
    5 | int foo() { return B; }
      |                    ^
Compiler returned: 1

But I think we should remove BOOST_CURRENT_FUNCTION because there may be collision if user includes proper boost header. I'll just replace BOOST_CURRENT_FUNCTION with TANGO_CURRENT_FUNCTION if that's ok with you.

TANGO_QUOTE must stay for the same reason.

@t-b
Copy link
Collaborator

t-b commented Aug 13, 2020

@mliszcz Thanks, learned something today.

I'll just replace BOOST_CURRENT_FUNCTION with TANGO_CURRENT_FUNCTION if that's ok with you.

Jeep, that is good!

This is needed to avoid conflicts during compilation of device servers
where boost/current_function.hpp is also included.
@t-b t-b self-requested a review August 27, 2020 09:49
@t-b t-b added the EasyToReview PRs which do not require wizard-level of kernel knowlegde label Oct 8, 2020
@sergirubio
Copy link

Hi all,
If you agree, I can do the review for this PR
is it ok?
Sergi

@mliszcz mliszcz requested a review from sergirubio October 14, 2020 08:51
@mliszcz
Copy link
Collaborator Author

mliszcz commented Oct 14, 2020

Thanks @sergirubio ! I just added you as a reviewer of this PR.

@sergirubio
Copy link

Hi Michal,

I already build your branch and I am going to test it using a modified TangoTest device compiled against it with -v4 flag on it.

I have just one question ... to see the information in the DevFailed exception from a client, the client should be also using the same branch or it could be a tango 9.3.4 python client?

Sergi

@mliszcz
Copy link
Collaborator Author

mliszcz commented Dec 10, 2020

Hi Sergi,
Thanks for checking this PR.

If the exception comes from the server, then you don't need the code in this branch on the client side. Location information is just appended to the origin field which is transferred from the server.

For exceptions throw from the client code, you'll not have code location information if you use 9.3.4.

@t-b t-b closed this Feb 24, 2022
@t-b t-b closed this Feb 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

EasyToReview PRs which do not require wizard-level of kernel knowlegde high priority Ready For Merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants